Re: [PATCH] Fix infinite recursion between store_fixed_bit_field/store_split_bit_field with STRICT_ALIGNMENT

[Richard Biener <richard dot guenther at gmail dot com>]

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.