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.
Richard.
