Pinging Eric.
On 11/23/2016 11:49 AM, Richard Biener wrote:
> On Wed, Nov 23, 2016 at 11:26 AM, Martin Liška <mli...@suse.cz> wrote:
>> Following patch fixes situation where we do a store to a bitfield which
>> is at boundary of a record. This leads to usage of wider store, leading
>> to overwriting a following memory location.
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>> Apart from that, the reported test-case in PR works on x86_64-linux-gnu.
>>
>> Ready to be installed?
>
> + HOST_WIDE_INT bitregion_end
> + = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1;
>
> I don't think looking at the CONSTRUCTOR to determine bitregion_end is
> a good idea.
> The function gets 'size' as argument which is documented as "number of
> bytes we are
> allowed to modify" - so better use that.
>
> @@ -6308,7 +6314,8 @@ store_constructor (tree exp, rtx target, int
> cleared, HOST_WIDE_INT size,
> MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;
> }
>
> - store_constructor_field (to_rtx, bitsize, bitpos, mode,
> + store_constructor_field (to_rtx, bitsize, bitpos,
> + 0, bitregion_end, mode,
> value, cleared,
> get_alias_set (TREE_TYPE (field)),
> reverse);
>
> this stores to to_rtx which may be offsetted from target this means in this
> case
> bitregion_end is not conservative enough - you'd need to resort to the
> field width
> in that case I guess (and for variable field size not specify any end
> -- I suppose
> the 'size' store_constructor gets might also be "unknown"?). But maybe all
> the non-constant offset / size cases are "dead code" now that we are in
> GIMPLE?
> Note they likely can only appear from Ada code anyway -- CCing Eric.
>
> I suppose a "safe" thing to do would be to give up on the first
> variable offset/size
> and re-set bitregion_end to zero for this and all following fields.
>
> The other cases look fine to me.
>
> Thanks,
> Richard.
>
>> Martin