On Mon, Mar 16, 2015 at 11:53 AM, Bernd Edlinger
<[email protected]> wrote:
>
> Hi,
>
>
> when looking at the m68k I realized the following, which is
> a general problem...
>
> If the alignment of the structure is less than sizeof(field), the
> strict volatile bitfields code may read beyond the end of the
> structure!
>
> Consider this example:
>
> struct s
> {
> char x : 8;
> volatile unsigned int y : 31;
> volatile unsigned int z : 1;
> } __attribute__((packed));
>
> struct s global;
>
>
> Here we have sizeof(struct s) = 5, alignment(global) == 1,
> However when we access global.z we read a 32-bit word
> at offset 4, which touches 3 bytes that are not safe to use.
>
> Something like that does never happen with -fno-strict-volatile-bitfields,
> because IIRC, with the only exception of the simple_mem_bitfield_p code path,
> there is never an access mode used which is larger than MEM_ALIGN(x).
Are you sure? There is still PR36043 ...
> In this example, if I want to use the packed attribute,
> I also have to use the aligned(4) attribute, this satisfies the
> check "MEM_ALIGN (op0) < modesize", which is IMO always necessary
> for strict volatile bitfields, not only on STRICT_ALIGNMENT targets.
But your patch still somehow special-cases them.
> On a target, that has BIGGEST_ALIGNMENT < BITS_PER_WORD,
> to use the strict volatile bitfields, you have to add the
> __attribute__((aligned(4)))
> to the structure.
>
> I had to do that on the pr23623.c test case, to have it passed on m68k for
> instance.
>
>
> I have attached the updated patch. As explained before, the check
> MEM_ALIGN (op0) < modesize should always be done in
> strict_volatile_bitfield_p.
>
> For the targets, that usually enable -fstrict-volatile-bitfields, nothing
> changes,
> Except when we use "packed" on the structure, we need to add also an
> aligned(4)
> attribute. For m68k where the natural alignment of any structure is <=2 we
> need to
> force aligned(4) if we want to ensure the access is in SImode.
>
> Boot-strapped and reg-tested on x86_64-linux-gnu.
> OK for trunk?
So - shouldn't the check be
if (MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (fieldmode))
return false;
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),
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 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).
Thanks,
Richard.
>
> Thanks
> Bernd.
>
