On Fri, Nov 15, 2013 at 12:38 PM, Bernd Edlinger
<[email protected]> wrote:
>>
>> Err, well. This just means that the generic C++ memory model
>> handling isn't complete. We do
>>
>> if (TREE_CODE (to) == COMPONENT_REF
>> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
>> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>>
>> and thus restrict ourselves to bitfields to initialize the region we may
>> touch.
>> This just lacks computing bitregion_start / bitregion_end for other
>> fields.
>>
>> Btw, what does the C++ memory model say for
>>
>> struct { char x; short a; short b; } a __attribute__((packed));
>>
>> short *p = &a.a;
>>
>> *p = 1;
>>
>
> this is not allowed. It should get at least a warning, that p is assigned
> a value, which violates the alignment restrictions.
> with packed structures you always have to access as "a.a".
>
>> I suppose '*p' may not touch bits outside of a.a either? Or are
>> memory accesses via pointers less constrained? What about
>> array element accesses?
>>
>
> of course, *p may not access anything else than a short.
> but the code generation may assume the pointer is aligned.
>
>> That is, don't we need
>>
>> else
>> {
>> bitregion_start = 0;
>> bitregion_end = bitsize;
>> }
>>
>> ?
>>
>> Richard.
>>
>
> That looks like always pretending it is a bit field.
> But it is not a bit field, and bitregion_start=bitregion_end=0
> means it is an ordinary value.
I don't think it is supposed to mean that. It's supposed to mean
"the access is unconstrained".
> It is just the bottom half of the expansion in expmed.c that does
> not handle that really well, most of that code seems to ignore the
> C++ memory model. For instance store_split_bit_field only handles
> bitregion_end and not bitregion_start. And I do not see, why it
> tries to write beyond a packed field at all. But that was OK in the past.
>
> And then there is this:
>
> if (get_best_mem_extraction_insn (&insv, EP_insv, bitsize, bitnum,
> fieldmode)
> && store_bit_field_using_insv (&insv, op0, bitsize, bitnum, value))
> return true;
>
> which dies not even care about bitregion_start/end.
Hopefully insv targets exactly match bitnum/bitsize.
> An look at that code in store_bit_field:
>
> if (MEM_P (str_rtx) && bitregion_start> 0)
> {
> enum machine_mode bestmode;
> HOST_WIDE_INT offset, size;
>
> gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);
>
> offset = bitregion_start / BITS_PER_UNIT;
> bitnum -= bitregion_start;
> size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
> bitregion_end -= bitregion_start;
> bitregion_start = 0;
> bestmode = get_best_mode (bitsize, bitnum,
> bitregion_start, bitregion_end,
> MEM_ALIGN (str_rtx), VOIDmode,
> MEM_VOLATILE_P (str_rtx));
> str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset,
> size);
> }
>
> I'd bet 1 Euro, that the person who wrote that, did that because it was too
> difficult and error-prone
> to re-write all the code below, and found it much safer to change the
> bitregion_start/end
> here instead.
It wasn't me ;)
> My patch is just layered on this if-block to accomplish the task. And that is
> why I think it is the
> right place here.
Well. You are looking at the alignment of str_rtx which I'm not sure
whether it's the correct alignment at this point (we at least start
with the alignment of the base).
+ /* Treat unaligned fields like bit regions otherwise we might
+ touch bits outside the field. */
+ if (MEM_P (str_rtx) && bitregion_start == 0 && bitregion_end == 0
+ && bitsize > 0 && bitsize % BITS_PER_UNIT == 0
bitsize is always > 0, why restrict it to byte-sized accesses?
+ && bitnum % BITS_PER_UNIT == 0
and byte-aligned accesses?
+ && (bitsize % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0
+ || bitnum % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0))
and what's this BITS_PER_WORD doing here? It seems what you
are testing for (and should say so in the comment) is non-power-of-two
size accesses or accesses that are not naturally aligned.
But as said, I fail to see why doing this here instead of at the
point we compute the initial bitregion_start/end is appropriate - you
are after all affecting callers such as
calls.c: store_bit_field (reg, bitsize, endian_correction, 0, 0,
and not only those that will possibly start from a non-zero bitregion_start/end.
Richard.
+ {
+ bitregion_start = bitnum;
+ bitregion_end = bitnum + bitsize - 1;
+ }
> Bernd.
