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

[Julian Brown <julian at codesourcery dot com>]

On Thu, 14 Nov 2013 11:09:22 +0100
Richard Biener <richard.guenther@gmail.com> wrote:

> 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.

Do you mean:

http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00082.html

I had overlooked that, thanks (it might have saved me some time!). IIUC
it looks like the main objection to the earlier stor-layout patch was
the potential for inadvertently changing the ABI on some given target,
rather than the fix being incorrect as such.

The patch resulting from PR57748 (comment #42) unfortunately doesn't
appear to solve this case (I'm not entirely sure if it was supposed
to: rs6000/E500 doesn't have a movmisalign insn).

So that leaves us with store_fixed_bit_field and store_split_bit_field
being unable to deal with accesses to packed structures with
non-BLKmode. I experimented earlier with a patch which merely worked
around the problem there (not thoroughly tested!):

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c        (revision 204350)
+++ gcc/expmed.c        (working copy)
@@ -962,6 +962,12 @@ store_fixed_bit_field (rtx op0, unsigned
        mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end,
                              MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));

+      if (mode == VOIDmode && STRICT_ALIGNMENT
+         && bitregion_start == 0 && bitregion_end == 0
+         && bitnum + bitsize <= GET_MODE_BITSIZE (word_mode))
+       mode = get_best_mode (bitsize, bitnum, bitregion_start, bitregion_end,
+                             MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+
       if (mode == VOIDmode)
        {
          /* The only way this should occur is if the field spans word

I.e. explicitly testing that the field does not span a word
boundary so that the comment in the following if statement remains
true. That felt like a hack to me, but would something along those lines
be more acceptable? If not, how should this be fixed?

Thanks,

Julian