| Message ID | 20131113152145.6066a109@octopus |
|---|---|
| State | New |
| Headers | show |
On Wed, 13 Nov 2013, Julian Brown wrote:
> * gcc.dg/packed-struct-mode-1.c: New.
I think this should be a torture test rather than specifying -O2, and
should declare malloc itself rather than using <stdlib.h>.
On Wed, Nov 13, 2013 at 4:21 PM, Julian Brown <julian@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.
Index: gcc/stor-layout.c =================================================================== --- gcc/stor-layout.c (revision 204674) +++ gcc/stor-layout.c (working copy) @@ -1601,6 +1601,7 @@ compute_record_mode (tree type) && integer_zerop (TYPE_SIZE (TREE_TYPE (field))))) || ! host_integerp (bit_position (field), 1) || DECL_SIZE (field) == 0 + || integer_zerop (DECL_SIZE (field)) || ! host_integerp (DECL_SIZE (field), 1)) return; Index: gcc/testsuite/gcc.dg/packed-struct-mode-1.c =================================================================== --- gcc/testsuite/gcc.dg/packed-struct-mode-1.c (revision 0) +++ gcc/testsuite/gcc.dg/packed-struct-mode-1.c (revision 0) @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-std=gnu99 -O2" } */ + +#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; +}
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? 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.