Bug 12786 – Crashes on arm due to different struct packing

• Component: New Bugs
• Status: RESOLVED FIXED

[Krzysztof Kowalczyk]

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.

[Krzysztof Kowalczyk]

Created attachment 13198 [details]
fix arm crashes due to different struct packing

[Oliver Hunt]

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]

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]

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]

My bad - you're right. Do you want me to update the patch or is it fine as is?

[Adam Roben (:aroben)]

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]

commited as is in r19679. I'll look into moving PACK_STRUCT definition to config.h in the future.