On Mon, Mar 30, 2015 at 9:42 PM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> 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).
We don't for
volatile struct s
{
unsigned x:15__attribute__((packed)) ;
} g __attribute__((aligned(4)));
>> 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.
Yes. bitregion_end should provide a good hint here?
>> 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?
Ok if nobody else complains within 24h.
Thanks,
Richard.
>
> Thanks,
> Bernd.
>
