Bug 28391 - Incorrect casts in JavaScriptCore with gcc-3.4
Summary: Incorrect casts in JavaScriptCore with gcc-3.4
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-08-17 08:44 PDT by Balazs Kelemen
Modified: 2009-08-22 08:26 PDT (History)
5 users (show)

See Also:

proposed patch (6.89 KB, patch)
2009-08-17 08:50 PDT, Balazs Kelemen
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2009-08-17 08:44:45 PDT
I tried to compiling ToT on the Maemo VM.
"gcc --version" says: sbox-arm-linux-gcc (GCC) 3.4.4 (release) (CodeSourcery ARM 2005q3-2)
There are reinterpret_cast-s in the JIT infrastructure what casts
void*-s to function pointers and vica versa. My compiler does not like this (I heard that the problem
is typical in gcc-3) so I get such errors: "ISO C++ forbids casting between pointer-to-function and pointer-to-object".
Comment 1 Balazs Kelemen 2009-08-17 08:50:37 PDT
Created attachment 34969 [details]
proposed patch

Using bitwise casts when reinterpret_cast leads to compile error.
Comment 2 Oliver Hunt 2009-08-18 11:42:03 PDT
I am tempted to r- this patch, as webkit requires gcc-4.x or higher to build so i'm surprised you can get beyond jsc ...
Comment 3 Eric Seidel (no email) 2009-08-21 10:57:00 PDT
Comment on attachment 34969 [details]
proposed patch

No your fault, but I went to look up WTF::bitwise_cast, only to find that the comment next to bitwise_cast is basically useless:

     * C++'s idea of a reinterpret_cast lacks sufficient cojones.
    template<typename TO, typename FROM>
    TO bitwise_cast(FROM from)
        COMPILE_ASSERT(sizeof(TO) == sizeof(FROM), WTF_bitwise_cast_sizeof_casted_types_is_equal);
        union {
            FROM from;
            TO to;
        } u;
        u.from = from;
        return u.to;

And I would expect that bitwise_cast violates the c++ standard, since I've been told that only the last union member you write to is valid to read from, any other member is undefined.  (I've also been told the GCC does define this behavior and that our use of unions here will work for GCC.   I'm anything bug a C++ standards expert though.)

I agree with oliver,  GCC 3.x is not supported for WebKit.  I don't think we should take one-off patches for it, since it's undoubtably going to break again.
Comment 4 Eric Seidel (no email) 2009-08-21 10:59:23 PDT
Looks like Gavin and Geoff wrote that comment. :)  http://trac.webkit.org/changeset/39958
Comment 5 Joe Mason 2009-08-21 11:31:00 PDT
This post seems to quote the relevant sections of the standard: http://learningcppisfun.blogspot.com/2008/07/c-unions-can-have-functions.html

It concludes that it's legal to do this cast as long as both members are POD types or structs containing only POD types with the same layout.  So whether this cast is legal or not depends on what FROM and TO are.
Comment 6 Gavin Barraclough 2009-08-21 15:04:11 PDT
My bad on the comment, I should make this more descriptive.  That said, I reassert my statement about reinterpret_cast lacking cojones. :-)

The real intention of the cast was to cast between POD types, (int64_t and double), so it sounds like this is safe, but I'll look into this more at some point.

If this is not safe, maybe we should more explicitly limit this (maybe we can do a kind of compile assert with a template specialization to guard against bitwise_cast being used with pointers, if that helps), or we could remove the generic cast from WTF and just make this an explicit cast within JSImmediate to prevent further use.  Or we can always implement this in inline asm, if absolutely necessary.  (Oh, c++, why are you so dumb).
Comment 7 Balazs Kelemen 2009-08-22 05:39:22 PDT
I did not think I would start such an interesting conversation :)
Anyway, my original wish (to be able to compile webkit with gcc-3.x) was not a valid conception, so I would close the bug. Should I close it, or you guys want to move on with the casting debate here? :-)
Comment 8 Eric Seidel (no email) 2009-08-22 08:26:19 PDT
Nah, we can close.  Thank you for the patch though!  And thanks to Gavin and Joe for following up with the C++ lesson. :)  I certainly learned something!