Bug 131

Summary: struct globals/struct globals2 not portable or safe
Product: Busybox Reporter: Peter Seebach <busybox-bugzilla>
Component: OtherAssignee: unassigned
Status: RESOLVED FIXED    
Severity: major CC: busybox-cvs, vda.linux
Priority: P3    
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Host: linux Target: 64-bit MIPS
Build:

Description Peter Seebach 2009-02-25 22:37:42 UTC 
It's possible (at least in 1.11) for t_double to end up misaligned, causing bus errors.

Underneath it all, the problem is that the trick of declaring two structures and shoving them next to each other in memory provides no alignment guarantees.  More generally, it's not a particularly well-designed strategy; there's not much likely benefit from "small offsets" when you're dealing with structures this large anyway, and the alignment penalties are probably worse.

The simplest fix would just be to use a single larger 'struct globals' and trust the compiler.  I have no reason to believe that this elaborate construct is saving a measurable amount of CPU time.  Alternatively, move t_double to the beginning of the first 'struct globals' so it forces suitable alignment.
Comment 1 Denys Vlasenko 2009-02-26 11:25:29 UTC 
> Underneath it all, the problem is that the trick of declaring two structures
and shoving them next to each other in memory provides no alignment guarantees.

> More generally, it's not a particularly well-designed strategy; there's not
much likely benefit from "small offsets" when you're dealing with structures
this large anyway,

In hindsight, I agree with you. This probably is a bit too ugly. However, size savings aren't too small - structures are not "large", one is < 128 bytes and other is barely bigger, so this ugly trick did result in almost all offsets being single-byte encodable on x86-32.

Try combining it into one struct, then compare code size, and you will see.

> and the alignment penalties are probably worse.

But this can be fixed:

http://busybox.net/downloads/fixes-1.13.2/busybox-1.13.2-awk.patch

Fixed in svn. Thanks.