On Thu, 11 Aug 2016, Bernd Edlinger wrote:
> On 08/11/16, Richard Biener wrote:
> >
> > The patch looks mostly ok, but
> >
> > + else
> > + {
> > + boff >>= LOG2_BITS_PER_UNIT;
> > + boff += tree_to_uhwi (component_ref_field_offset (ref));
> > + coff = size_binop (MINUS_EXPR, coff, ssize_int (boff));
> >
> > how can we be sure that component_ref_field_offset is an INTEGER_CST?
> > At least Ada can have variably-length fields before a bitfield. We'd
> > need to apply component_ref_field_offset to off in that case. Which
> > makes me wonder if we can simply avoid the COMPONENT_REF path in
> > a first iteration of the patch and always build a BIT_FIELD_REF?
> > It should solve the correctness issues as well and be more applicable
> > for branches.
> >
>
> Oh yes, thanks for catching that!
>
> If that information is true, that ought to go into the comment before
> the if, that would certainly be an interesting comment :-)
>
> Are there any test cases for this non-constant field offsets?
>
> I see many checks if TREE_TYPE of
> component_ref_field_offset is INTEGER_CST, but with very little
> background why it could be otherwise.
>
> I think we should simply fall back to the BIT_FIELD_REF in that case,
> that would mean, the if should be something like:
>
> tree offset = component_ref_field_offset (ref);
> if (boff % BITS_PER_UNIT != 0
> || !tree_fits_uhwi_p (offset))
>
> And yes, the correctness issue can certainly be solved with the
> BIT_FIELD_REF alone.
>
> So, as requested, here is a first iteration of my patch that always builds
> a BIT_FIELD_REF, together with the test cases.
>
>
> Boot-strap & regression testing was done on x86_64-pc-linux-gnu.
> Is it OK for trunk (and active branches)?
Yes.
Thanks,
Richard.
