On Wed, Nov 13, 2013 at 4:21 PM, Julian Brown <jul...@codesourcery.com> wrote: > Hi, > > This patch addresses an issue where the compiler gets stuck in an > infinite mutually-recursive loop between store_fixed_bit_field and > store_split_bit_field. This affects versions back at least as far as > 4.6 (or so). We observed this happening on PowerPC E500, but other > targets may be affected too. (The symptom is the same as PR 55438, but > the cause is different.) > > A small testcase is as follows (compile with a toolchain targeting > "powerpc-linux-gnuspe" and configured with "--with-cpu=8548", > currently requiring minor hacks to work around e.g. libsanitizer > breakage): > > #include <stdlib.h> > > typedef struct { > char pad; > int arr[0]; > } __attribute__((packed)) str; > > str * > foo (int* src) > { > str *s = malloc (sizeof (str) + sizeof (int)); > s->arr[0] = 0x12345678; > return s; > } > > $ powerpc-linux-gnuspe-gcc -O2 -c min.c > (Segfault) > > The problem is as follows: in stor-layout.c:compute_record_mode, the > record (struct) "str" is considered to have a single element (the "char > pad"), since only the size is checked and not the elements themselves: > as an optimisation the record as a whole is given the mode of the first > element, since that fits nicely into a machine word and then (the idea > is that) the record can be held in a register. In this case, the mode > given will be QImode. > > Now, E500 cores cannot handle misaligned data accesses, at least for > some subset of instructions (STRICT_ALIGNMENT is true on such cores), so > accessing elements of the array "arr" in the packed structure will > typically use read-modify-write operations. > > The function expmed.c:store_fixed_bit_field uses get_best_mode to try > to find a suitable mode for that read-modify-write operation: the > mode passed into get_best_mode is taken from op0 (inside the "if (MEM_P > (op0))" clause). Because the record type we are accessing has > QImode, this looks something like: > > (mem:QI (reg:SI ...)) > > Now stor-layout.c:bit_field_iterator::next_mode will reject any mode > which is smaller than the size of the access we want to do (32 bits, or > 24 bits after store_split_bit_field has been called once), skipping over > QImode and HImode. The SImode value returned is then rejected in > get_best_mode because it is bigger than largest_mode, which is QImode > (from before), so it returns VOIDmode. > > That means that store_split_bit_field is called (from > store_fixed_bit_field), but now the damage has been done: we still have > a MEM for op0, so the "else" clause "word = op0" is executed, and we > recurse back into store_fixed_bit_field at the end of the function, and > we're back where we started -- this leads to infinite recursion between > those two functions, which eventually blows up the stack and crashes > the compiler. > > Anyway: the short story is that a record that finishes with a > zero-length array should never be given the mode of its > "only" (non-zero-sized) element to start with. The attached patch stops > that from happening. (A flexible trailing array member, "int arr[];" is > handled correctly -- left as BLKmode -- due to the existing "DECL_SIZE > (field) == 0" check.) > > Tested (gcc/g++/libstdc++) with an E500 cross-compiler as configured > above. The newly-added test fails without the patch, and passes with. OK > to apply, or any comments?
See the large other thread with zero-sized arrays and why a stor-layout.c fix doesn't really fix the underlying issue. Richard. > Thanks, > > Julian > > ChangeLog > > gcc/ > * stor-layout.c (compute_record_mode): Handle zero-sized array > members. > > gcc/testsuite/ > * gcc.dg/packed-struct-mode-1.c: New.