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?
Is the patch OK as-is? 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.
Please help properly documenting this API.
Thanks,
Richard.
> Martin
>
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > OK for trunk and branches?
> >
> > Thanks,
> > Richard.
> >
> > 2022-05-16 Richard Biener <rguent...@suse.de>
> >
> > PR middle-end/105604
> > * gimple-ssa-sprintf.cc (get_origin_and_offset_r):
> > Handle non-constant ARRAY_REF element size and non-constant
> > COMPONENT_REF field offset.
> >
> > * gcc.dg/torture/pr105604.c: New testcase.
> > ---
> > gcc/gimple-ssa-sprintf.cc | 14 +++++++++++---
> > gcc/testsuite/gcc.dg/torture/pr105604.c | 24 ++++++++++++++++++++++++
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.dg/torture/pr105604.c
> >
> > diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc
> > index c93f12f90b5..14e215ce69c 100644
> > --- a/gcc/gimple-ssa-sprintf.cc
> > +++ b/gcc/gimple-ssa-sprintf.cc
> > @@ -2312,14 +2312,16 @@ get_origin_and_offset_r (tree x, HOST_WIDE_INT
> > *fldoff, HOST_WIDE_INT *fldsize,
> > HOST_WIDE_INT idx = (tree_fits_uhwi_p (offset)
> > ? tree_to_uhwi (offset) : HOST_WIDE_INT_MAX);
> > + tree elsz = array_ref_element_size (x);
> > tree eltype = TREE_TYPE (x);
> > if (TREE_CODE (eltype) == INTEGER_TYPE)
> > {
> > if (off)
> > *off = idx;
> > }
> > - else if (idx < HOST_WIDE_INT_MAX)
> > - *fldoff += idx * int_size_in_bytes (eltype);
> > + else if (idx < HOST_WIDE_INT_MAX
> > + && tree_fits_shwi_p (elsz))
> > + *fldoff += idx * tree_to_shwi (elsz);
> > else
> > *fldoff = idx;
> > @@ -2350,8 +2352,14 @@ get_origin_and_offset_r (tree x, HOST_WIDE_INT
> > *fldoff, HOST_WIDE_INT *fldsize,
> >
> > case COMPONENT_REF:
> > {
> > + tree foff = component_ref_field_offset (x);
> > tree fld = TREE_OPERAND (x, 1);
> > - *fldoff += int_byte_position (fld);
> > + if (!tree_fits_shwi_p (foff)
> > + || !tree_fits_shwi_p (DECL_FIELD_BIT_OFFSET (fld)))
> > + return x;
> > + *fldoff += (tree_to_shwi (foff)
> > + + (tree_to_shwi (DECL_FIELD_BIT_OFFSET (fld))
> > + / BITS_PER_UNIT));
> >
> > get_origin_and_offset_r (fld, fldoff, fldsize, off);
> > x = TREE_OPERAND (x, 0);
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr105604.c
> > b/gcc/testsuite/gcc.dg/torture/pr105604.c
> > new file mode 100644
> > index 00000000000..b002251df10
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr105604.c
> > @@ -0,0 +1,24 @@
> > +/* { dg-do compile } */
> > +/* { dg-additional-options "-Wall" } */
> > +
> > +struct {
> > + long users;
> > + long size;
> > + char *data;
> > +} * main_trans;
> > +void *main___trans_tmp_1;
> > +int sprintf(char *, char *, ...);
> > +int main() {
> > + int users = 0;
> > + struct {
> > + long users;
> > + long size;
> > + char *data;
> > + int links[users];
> > + char buf[];
> > + } *trans = trans;
> > + trans->data = trans->buf;
> > + main___trans_tmp_1 = trans;
> > + main_trans = main___trans_tmp_1;
> > + sprintf(main_trans->data, "test");
> > +}
>
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
