On Mon, 23 May 2022, Martin Sebor wrote:
> On 5/19/22 05:39, Richard Biener wrote:
> > On Wed, 18 May 2022, Martin Sebor wrote:
> >
> >> On 5/18/22 00:26, Richard Biener wrote:
> >>> On Tue, 17 May 2022, Martin Sebor wrote:
> >>>
> >>>> On 5/16/22 03:16, Richard Biener wrote:
> >>>>> The following tries to correct get_origin_and_offset_r not handling
> >>>>> non-constant sizes of array elements in ARRAY_REFs and non-constant
> >>>>> offsets of COMPONENT_REFs. It isn't exactly clear how such failures
> >>>>> should be treated in this API and existing handling isn't consistent
> >>>>> here either. The following applies two different variants, treating
> >>>>> non-constant array sizes like non-constant array indices and
> >>>>> treating non-constant offsets of COMPONENT_REFs by terminating
> >>>>> the recursion (not sure what that means to the callers).
> >>>>>
> >>>>> Basically the code failed to use component_ref_field_offset and
> >>>>> array_ref_element_size and instead relies on inappropriate
> >>>>> helpers (that shouldn't exist in the first place ...). The code
> >>>>> is also not safe-guarded against overflows in the final offset/size
> >>>>> computations but I'm not trying to rectify that.
> >>>>>
> >>>>> Martin - can you comment on how the API should handle such
> >>>>> situations?
> >>>>
> >>>> It looks like the -Wrestrict warning here ignores offsets equal to
> >>>> HOST_WIDE_INT_MIN so presumably setting dst_offset (via *fldoff) to
> >>>> that should avoid it. Or maybe to HWI_MAX as it does for variable
> >>>> offsets.
> >>>
> >>> Can you suggest wording for the function comment as to how it handles
> >>> the case when offset or size cannot be determined exactly? The
> >>> comment currently only suggests that the caller possibly cannot
> >>> trust fldsize or off when the function returns NULL but the actual
> >>> implementation differs from that.
> >>
> >>
> >>
> >>>
> >>>> It also looks like the function only handles constant offsets and
> >>>> sizes, and I have a vague recollection of enhancing it to work with
> >>>> ranges. That should avoid the overflow problem too.
> >>>
> >>> So the correct thing is to return NULL?
> >>
> >> No, I don't think so. The recursive get_origin_and_offset_r() assumes
> >> its own invocations never return null (the one place it does that should
> >> probably be moved to the nonrecursive caller).
> >>
> >>>
> >>> Is the patch OK as-is?
> >>
> >> It's an improvement but it's not complete as the following also ICEs
> >> (albeit somewhere else):
> >>
> >> void* f (void);
> >>
> >> void g (int n)
> >> {
> >> struct {
> >> char a[n], b[];
> >> } *p = f ();
> >>
> >> __builtin_sprintf (p->b, "%s", p->a);
> >> }
> >>
> >> With the ICE fixed the warning triggers. That's not ideal but it's
> >> unavoidable given the IR (I believe I mentioned this caveat some time
> >> back). This is the same as for:
> >>
> >> struct {
> >> char a[8], b[8];
> >> } *p = f ();
> >>
> >> __builtin_sprintf (&p->b[n], "%s", p->a);
> >>
> >> because the IR looks more or less the same for &p->a[n] as it is for
> >> &p->b[n].
> >>
> >>> As said, I'm not sure how the caller interprets
> >>> the result and how it can distinguish the exact vs. non-exact cases
> >>> or what a "conservative" inexact answer would be.
> >>
> >> The warning triggers in both the certain cases and the inexact
> >> ones like the one above when an overlap cannot be ruled out. To
> >> differentiate the two it's phrased as "may overlap". The handling
> >> is in maybe_warn_overlap().
> >>
> >>>
> >>> Please help properly documenting this API.
> >>
> >> I can spend some time in the next few days to page it all in, see
> >> if I can clean it up a bit in addition to fixing the ICEs and
> >> improve the comment. Let me know if you have a different
> >> preference.
> >
> > That works for me - thanks for taking it from here.
> Attached is a slightly enhanced patch that fixes both of the ICEs,
> improves the comments, and adds more tests. I tested it on x86_64.
> Let me know if there's something else you'd like me to do here.
Looks good to me!
Thanks for fixing.
Richard.
