Hi, On Mon, 30 Mar 2015 12:33:47, Richard Biener wrote: > > So - shouldn't the check be > > if (MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (fieldmode)) > return false; >
No. Because this example would access memory beyond the end of structure
on m66k:
volatile struct s
{
unsigned x:15;
} g;
int x = g.x;
but when MEM_ALIGN(op0) == fieldmode we know that
sizeof(struct s) == sizeof(int).
> instead? And looking at
>
> /* Check for cases of unaligned fields that must be split. */
> - if (bitnum % BITS_PER_UNIT + bitsize> modesize
> - || (STRICT_ALIGNMENT
> - && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize> modesize))
> + if (bitnum % (STRICT_ALIGNMENT ? modesize : BITS_PER_UNIT)
> + + bitsize> modesize)
> return false;
>
> I wonder what the required semantics are - are they not that we need to access
> the whole "underlying" field with the access (the C++ memory model only
> requires we don't go beyond the field)? It seems that information isn't
> readily
> available here though. So the check checks that we can access the field
> with a single access using fieldmode. Which means (again),
>
And another requirement must of course be, that we should never read anything
outside of the structure's limits.
> if (bitnum % (STRICT_ALIGNMENT ? GET_MODE_ALIGNMENT (fieldmode) :
> BITS_PER_UNIT)
> + bitsize> modesize)
>
> Also this means that for !STRICT_ALIGNMENT platforms the MEM_ALIGN check isn't
> sufficient which is what the other hunks in the patch are about to fix?
>
> It seems at least the
>
> @@ -988,6 +995,16 @@ store_bit_field (rtx str_rtx, unsigned HOST_WIDE_I
>
> str_rtx = narrow_bit_field_mem (str_rtx, fieldmode, bitsize, bitnum,
> &bitnum);
> + if (!STRICT_ALIGNMENT
> + && bitnum + bitsize> GET_MODE_BITSIZE (fieldmode))
> + {
> + unsigned HOST_WIDE_INT offset;
> + offset = (bitnum + bitsize + BITS_PER_UNIT - 1
> + - GET_MODE_BITSIZE (fieldmode)) / BITS_PER_UNIT;
> + str_rtx = adjust_bitfield_address (str_rtx, fieldmode, offset);
> + bitnum -= offset * BITS_PER_UNIT;
> + }
> + gcc_assert (bitnum + bitsize <= GET_MODE_BITSIZE (fieldmode));
>
> hunks could do with a comment.
>
> That said, I fail to realize how the issue is specific to
> strict-volatile bitfields?
>
I will not try to defend this hunk at the moment, because it is actually not
needed to
fix the wrong code bug,
> I also hope somebody else will also look at the patch - I'm not feeling like
> the
> appropriate person to review changes in this area (even if I did so in
> the past).
>
Sorry, but I just have to continue...
So, now I removed these unaligned access parts again from the patch,
Attached you will find the new reduced version of the patch.
Boot-strapped and regression-tested on x86_64-linux-gnu.
OK for trunk?
Thanks,
Bernd.
gcc:
2015-03-30 Bernd Edlinger <bernd.edlin...@hotmail.de>
* expmed.c (strict_volatile_bitfield_p): Check that the access will
not cross a MODESIZE boundary.
(store_bit_field, extract_bit_field): Added assertions in the
strict volatile bitfields code path.
testsuite:
2015-03-30 Bernd Edlinger <bernd.edlin...@hotmail.de>
* gcc.dg/pr23623.c: Added aligned attribute.
* gcc.dg/20141029-1.c: Likewise.
* gcc.dg/20150306-1.c: New test.
patch-volatile-bitfields-1.diff
Description: Binary data
