RESOLVED FIXED 12786
Crashes on arm due to different struct packing
https://bugs.webkit.org/show_bug.cgi?id=12786
Summary Crashes on arm due to different struct packing
Krzysztof Kowalczyk
Reported 2007-02-15 23:53:34 PST
On some versions of ARM gcc, default struct packing is 4 bytes, which is different than default 1 byte packing on most archs. This causes problems (crashes) in some places. While it's possible to control struct packing via gcc command-line options, it's not feasible if the code accesses any OS stuff, because there would be mismatch in how structures are interpreted by the code. Since we can't recompile the OS, we have to pack some vital structures in webcore code.
Attachments
fix arm crashes due to different struct packing (4.79 KB, patch)
2007-02-15 23:54 PST, Krzysztof Kowalczyk
oliver: review-
updated patch (5.26 KB, patch)
2007-02-16 23:28 PST, Krzysztof Kowalczyk
aroben: review+
Krzysztof Kowalczyk
Comment 1 2007-02-15 23:54:42 PST
Created attachment 13198 [details] fix arm crashes due to different struct packing
Oliver Hunt
Comment 2 2007-02-16 22:48:25 PST
Comment on attachment 13198 [details] fix arm crashes due to different struct packing I'd be tempted to make it #if PLATFORM(ARM) (do we have that?) and if all possible move the compileassertion (and the Assertion.h include into a C file (we're already taking a long time to copile, aroben has been attempting to reduce the number of includes in headers) Also, it might be worth putting the +#if COMPILER(GCC) +#define PACK_STRUCT __attribute__((packed)) +#else +#define PACK_STRUCT +#endif into config.h
Krzysztof Kowalczyk
Comment 3 2007-02-16 23:28:25 PST
Created attachment 13207 [details] updated patch As per review comments: * changed the test to #if COMPILER(GCC) && PLATFORM(ARM) * move COMPILE_ASSERT to corresponding .cpp files to avoid #including Assertions.h in *.h files I don't think putting the PACK_STRUCT definition to config.h would be good since it would require #including "config.h" in ustring.h and DeprecatedString.h (at the moment no *.h files includes "config.h"). Platform.h maybe? But that would require #including "Platform.h" in ustring.h and DeprecatedString.h i.e. potentially longer compile times.
Oliver Hunt
Comment 4 2007-02-17 00:11:00 PST
Defining it in PACK_STRUCT in config.h is safe as *all* c/cpp files should have config.h as the first include
Krzysztof Kowalczyk
Comment 5 2007-02-17 00:14:54 PST
My bad - you're right. Do you want me to update the patch or is it fine as is?
Adam Roben (:aroben)
Comment 6 2007-02-17 00:52:15 PST
Comment on attachment 13207 [details] updated patch I don't think PACK_STRUCT absolutely needs to be in config.h, although it would probably be a good place for it to live. r=me.
Krzysztof Kowalczyk
Comment 7 2007-02-17 01:19:07 PST
commited as is in r19679. I'll look into moving PACK_STRUCT definition to config.h in the future.
Note You need to log in before you can comment on or make changes to this bug.