On Thu, Nov 14, 2013 at 1:08 PM, Julian Brown <jul...@codesourcery.com> wrote:
> On Thu, 14 Nov 2013 11:09:22 +0100
> Richard Biener <richard.guent...@gmail.com> wrote:
>
>> 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.
>
> 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!):
Unfortunately I don't have time to think about this (and the other) issue
thoroughly. I'll come back during stage3 I suppose. Maybe Eric
has some ideas.
Meanwhile please make sure to open a bugreport.
Richard.
> 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
