RE: [PATCH] Fix another wrong-code bug with -fstrict-volatile-bitfields

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Hi,

On Sat, 14 Mar 2015 13:24:33, Mikael Pettersson wrote:
>
> Bernd Edlinger writes:
>> Hi,
>>
>> are there any more comments on this?
>>
>> I would like to apply the patch as is, unless we find a
>> a way to get to a test case, maybe with a cross-compiler,
>> where the MODE_ALIGNMENT is different from MODE_BITSIZE.
>>
>> Currently, I think that does not happen.
>
> On m68k-linux GET_MODE_ALIGNMENT (SImode) == 16 while
> GET_MODE_BITSIZE (SImode) == 32.
>
> I don't know what that means for your patch, just wanted
> to inform you that such targets do exist.
>

Oh I see, thanks.

This is due to BIGGEST_ALIGNMENT=16, STRICT_ALIGNMENT=1 by default on
that architecture.

If I change this check as suggested:

  if (bitnum % (STRICT_ALIGNMENT ? GET_MODE_ALIGNMENT (fieldmode) : BITS_PER_UNIT)
      + bitsize> modesize
      || (STRICT_ALIGNMENT && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (fieldmode)))
    return false;


Then I can get the assertion failed in store_bit_field:
  gcc_assert (bitnum + bitsize <= GET_MODE_BITSIZE (fieldmode));

With this example:

cat test.c
struct s
{
  short y:16;
  int x:31;
};

void
f(volatile struct s* z, int x)
{
  z->x=x;
}

m68k-elf-gcc -fstrict-volatile-bitfields -mstrict-align -mno-align-int -O2 -S test.c
test.c: In function 'f':
test.c:10:7: internal compiler error: in store_bit_field, at expmed.c:1005
   z->x=x;
       ^

what I said before..,

without the patch the test case generates
just invalid code which assigns only 16 bits.

There is also a problem in this check.  I had to make
short y:16 a bit filed to bypass that, initially I wrote short y;

  /* Check for cases where the C++ memory model applies.  */
  if (bitregion_end != 0
      && (bitnum - bitnum % modesize < bitregion_start
          || bitnum - bitnum % modesize + modesize - 1> bitregion_end))
    return false;

This assumes also that the access is at a modesize boundary.

If we have BIGGEST_ALIGNMENT=16 that means we have likely a 16 bit
architecture. I doubt that the strict alignment code makes any sense for
modesize> BIGGEST_ALIGNMENT.

I think I should change this check

  /* The bit size must not be larger than the field mode, and
     the field mode must not be larger than a word.  */
  if (bitsize> modesize || modesize> BITS_PER_WORD)
    return false;

to this:

  if (bitsize> modesize || modesize> BITS_PER_WORD
      || modesize> BIGGEST_ALIGNMENT)
    return false;

This should avoid these oddities.


Bernd.