[PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Richard Biener]

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?

Bootstrapped and tested on x86_64-unknown-linux-gnu.

OK for trunk and branches?

Thanks,
Richard.

2022-05-16  Richard Biener  <rguenther@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");
+}
-- 
2.35.3

Re: [PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Martin Sebor]

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.

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.

Martin

> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> OK for trunk and branches?
> 
> Thanks,
> Richard.
> 
> 2022-05-16  Richard Biener  <rguenther@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");
> +}

Re: [PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Richard Biener]

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  <rguenther@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 <rguenther@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)

Re: [PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Martin Sebor]

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.

Martin

> 
> Thanks,
> Richard.
> 
>> Martin
>>
>>>
>>> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>>>
>>> OK for trunk and branches?
>>>
>>> Thanks,
>>> Richard.
>>>
>>> 2022-05-16  Richard Biener  <rguenther@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");
>>> +}
>>
>>
>>
> 

Re: [PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Richard Biener]

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.

Re: [PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 3879 bytes --]

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.

Martin

[-- Attachment #2: gcc-105604.diff --]
[-- Type: text/x-patch, Size: 16458 bytes --]

PR middle-end/105604 - ICE: in tree_to_shwi with vla in struct and sprintf

gcc/ChangeLog:

	PR middle-end/105604
	* gimple-ssa-sprintf.cc (set_aggregate_size_and_offset): Add comments.
	(get_origin_and_offset_r): Remove null handling.  Handle variable array
	sizes.
	(get_origin_and_offset): Handle null argument here.  Simplify.
	(alias_offset):
	* pointer-query.cc (field_at_offset): Update comment.

gcc/testsuite/ChangeLog:

	PR middle-end/105604
	* gcc.dg/Wrestrict-24.c: New test.
	* gcc.dg/Wrestrict-25.c: New test.
	* gcc.dg/Wrestrict-26.c: New test.

diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc
index 8202129667e..6bd27302213 100644
--- a/gcc/gimple-ssa-sprintf.cc
+++ b/gcc/gimple-ssa-sprintf.cc
@@ -2232,8 +2232,9 @@ format_character (const directive &dir, tree arg, pointer_query &ptr_qry)
 }
 
 /* If TYPE is an array or struct or union, increment *FLDOFF by the starting
-   offset of the member that *OFF point into and set *FLDSIZE to its size
-   in bytes and decrement *OFF by the same.  Otherwise do nothing.  */
+   offset of the member that *OFF points into if one can be determined and
+   set *FLDSIZE to its size in bytes and decrement *OFF by the same.
+   Otherwise do nothing.  */
 
 static void
 set_aggregate_size_and_offset (tree type, HOST_WIDE_INT *fldoff,
@@ -2249,9 +2250,9 @@ set_aggregate_size_and_offset (tree type, HOST_WIDE_INT *fldoff,
       if (array_elt_at_offset (type, *off, &index, &arrsize))
 	{
 	  *fldoff += index;
-	  *off -= index;
 	  *fldsize = arrsize;
 	}
+      /* Otherwise leave *FLDOFF et al. unchanged.  */
     }
   else if (RECORD_OR_UNION_TYPE_P (type))
     {
@@ -2269,11 +2270,12 @@ set_aggregate_size_and_offset (tree type, HOST_WIDE_INT *fldoff,
 	  *fldoff += index;
 	  *off -= index;
 	}
+      /* Otherwise leave *FLDOFF et al. unchanged.  */
     }
 }
 
-/* For an expression X of pointer type, recursively try to find the same
-   origin (object or pointer) as Y it references and return such a Y.
+/* For an expression X of pointer type, recursively try to find its origin
+   (either object DECL or pointer such as PARM_DECL) Y and return such a Y.
    When X refers to an array element or struct member, set *FLDOFF to
    the offset of the element or member from the beginning of the "most
    derived" object and *FLDSIZE to its size.  When nonnull, set *OFF to
@@ -2284,9 +2286,6 @@ static tree
 get_origin_and_offset_r (tree x, HOST_WIDE_INT *fldoff, HOST_WIDE_INT *fldsize,
 			 HOST_WIDE_INT *off)
 {
-  if (!x)
-    return NULL_TREE;
-
   HOST_WIDE_INT sizebuf = -1;
   if (!fldsize)
     fldsize = &sizebuf;
@@ -2308,23 +2307,33 @@ get_origin_and_offset_r (tree x, HOST_WIDE_INT *fldoff, HOST_WIDE_INT *fldsize,
 
     case ARRAY_REF:
       {
-	tree offset = TREE_OPERAND (x, 1);
-	HOST_WIDE_INT idx = (tree_fits_uhwi_p (offset)
-			     ? tree_to_uhwi (offset) : HOST_WIDE_INT_MAX);
+	tree sub = TREE_OPERAND (x, 1);
+	unsigned HOST_WIDE_INT idx =
+	  tree_fits_uhwi_p (sub) ? tree_to_uhwi (sub) : HOST_WIDE_INT_MAX;
 
-	tree eltype = TREE_TYPE (x);
-	if (TREE_CODE (eltype) == INTEGER_TYPE)
+	tree elsz = array_ref_element_size (x);
+	unsigned HOST_WIDE_INT elbytes =
+	  tree_fits_shwi_p (elsz) ? tree_to_shwi (elsz) : HOST_WIDE_INT_MAX;
+
+	unsigned HOST_WIDE_INT byteoff = idx * elbytes;
+
+	if (byteoff < HOST_WIDE_INT_MAX
+	    && elbytes < HOST_WIDE_INT_MAX
+	    && byteoff / elbytes == idx)
 	  {
+	    /* For in-bounds constant offsets into constant-sized arrays
+	       bump up *OFF, and for what's likely arrays or structs of
+	       arrays, also *FLDOFF, as necessary.  */
 	    if (off)
-	      *off = idx;
+	      *off += byteoff;
+	    if (elbytes > 1)
+	      *fldoff += byteoff;
 	  }
-	else if (idx < HOST_WIDE_INT_MAX)
-	  *fldoff += idx * int_size_in_bytes (eltype);
 	else
-	  *fldoff = idx;
+	  *fldoff = HOST_WIDE_INT_MAX;
 
 	x = TREE_OPERAND (x, 0);
-	return get_origin_and_offset_r (x, fldoff, fldsize, nullptr);
+	return get_origin_and_offset_r (x, fldoff, fldsize, off);
       }
 
     case MEM_REF:
@@ -2350,8 +2359,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);
@@ -2411,30 +2426,25 @@ get_origin_and_offset_r (tree x, HOST_WIDE_INT *fldoff, HOST_WIDE_INT *fldsize,
   return x;
 }
 
-/* Nonrecursive version of the above.  */
+/* Nonrecursive version of the above.
+   The function never returns null unless X is null to begin with.  */
 
 static tree
 get_origin_and_offset (tree x, HOST_WIDE_INT *fldoff, HOST_WIDE_INT *off,
 		       HOST_WIDE_INT *fldsize = nullptr)
 {
+  if (!x)
+    return NULL_TREE;
+
   HOST_WIDE_INT sizebuf;
   if (!fldsize)
     fldsize = &sizebuf;
 
+  /* Invalidate *FLDSIZE.  */
   *fldsize = -1;
+  *fldoff = *off = 0;
 
-  *fldoff = *off = *fldsize = 0;
-  tree orig = get_origin_and_offset_r (x, fldoff, fldsize, off);
-  if (!orig)
-    return NULL_TREE;
-
-  if (!*fldoff && *off == *fldsize)
-    {
-      *fldoff = *off;
-      *off = 0;
-    }
-
-  return orig;
+  return get_origin_and_offset_r (x, fldoff, fldsize, off);
 }
 
 /* If ARG refers to the same (sub)object or array element as described
@@ -2454,7 +2464,8 @@ alias_offset (tree arg, HOST_WIDE_INT *arg_size,
     return HOST_WIDE_INT_MIN;
 
   /* The two arguments may refer to the same object.  If they both refer
-     to a struct member, see if the members are one and the same.  */
+     to a struct member, see if the members are one and the same.  If so,
+     return the offset into the member.  */
   HOST_WIDE_INT arg_off = 0, arg_fld = 0;
 
   tree arg_orig = get_origin_and_offset (arg, &arg_fld, &arg_off, arg_size);
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 646606e6344..b7a6fb78f83 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -2448,9 +2448,13 @@ field_at_offset (tree type, tree start_after, HOST_WIDE_INT off,
       /* The offset of FLD within its immediately enclosing structure.  */
       HOST_WIDE_INT fldpos = next_pos < 0 ? int_byte_position (fld) : next_pos;
 
+      tree typesize = TYPE_SIZE_UNIT (fldtype);
+      if (typesize && TREE_CODE (typesize) != INTEGER_CST)
+	/* Bail if FLD is a variable length member.  */
+	return NULL_TREE;
+
       /* If the size is not available the field is a flexible array
 	 member.  Treat this case as success.  */
-      tree typesize = TYPE_SIZE_UNIT (fldtype);
       HOST_WIDE_INT fldsize = (tree_fits_uhwi_p (typesize)
 			       ? tree_to_uhwi (typesize)
 			       : off);
@@ -2464,7 +2468,11 @@ field_at_offset (tree type, tree start_after, HOST_WIDE_INT off,
 	{
 	  /* If OFF is equal to the offset of the next field continue
 	     to it and skip the array/struct business below.  */
-	  next_pos = int_byte_position (next_fld);
+	  tree pos = byte_position (next_fld);
+	  if (!tree_fits_shwi_p (pos))
+	    /* Bail if NEXT_FLD is a variable length member.  */
+	    return NULL_TREE;
+	  next_pos = tree_to_shwi (pos);
 	  *nextoff = *fldoff + next_pos;
 	  if (*nextoff == off && TREE_CODE (type) != UNION_TYPE)
 	    continue;
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-24.c b/gcc/testsuite/gcc.dg/Wrestrict-24.c
new file mode 100644
index 00000000000..d224d80f87a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wrestrict-24.c
@@ -0,0 +1,35 @@
+/* PR tree-optimization/105604  - ICE: in tree_to_shwi with vla in struct
+   and sprintf
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wrestrict" } */
+
+extern int sprintf (char*, const char*, ...);
+
+extern void* sink (void*, ...);
+
+struct {
+  long users;
+  long size;
+  char *data;
+} * main_trans;
+
+void *main___trans_tmp_1;
+
+int users = 0;
+
+void test (void)
+{
+  struct {
+    long users;
+    long size;
+    char *data;
+    int links[users];
+    char buf[];
+  } *trans = sink (0);
+
+  trans->data = trans->buf;
+  main___trans_tmp_1 = trans;
+  main_trans = main___trans_tmp_1;
+  sprintf (main_trans->data, "test");
+  sink (main_trans->data);
+}
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-25.c b/gcc/testsuite/gcc.dg/Wrestrict-25.c
new file mode 100644
index 00000000000..a15f56d7424
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wrestrict-25.c
@@ -0,0 +1,165 @@
+/* PR tree-optimization/105604  - ICE: in tree_to_shwi with vla in struct
+   and sprintf
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wrestrict" } */
+
+extern int sprintf (char*, const char*, ...);
+
+void* sink (void*);
+
+
+void sprintf_S_a8_an_bn (int n, int i, int j)
+{
+  struct {
+    char a8[8], an[n], bn[n];
+  } *p = sink (0);
+
+  {
+    char *d = p->a8 + i;
+    char *s = p->a8;
+    sprintf (d, "%s", s);       // { dg-warning "argument 3 may overlap" }
+    sink (p);
+  }
+
+  {
+    char *d = p->a8;
+    char *s = p->a8 + j;
+    sprintf (d, "%s", s);       // { dg-warning "argument 3 may overlap" }
+    sink (p);
+  }
+
+  {
+    char *d = p->a8 + i;
+    char *s = p->a8 + j;
+    sprintf (d, "%s", s);       // { dg-warning "argument 3 may overlap" }
+    sink (p);
+  }
+
+  {
+    char *d = p->a8 + i;
+    char *s = p->an;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->a8;
+    char *s = p->an + j;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->a8 + i;
+    char *s = p->an + j;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    /* The IL makes it impossible to rule out an overlap between
+       p->a8 + i and p->bn + i so the "may overlap" warning triggers.  */
+    char *d = p->a8 + i;
+    char *s = p->bn;
+    sprintf (d, "%s", s);       // { dg-bogus "-Wrestrict" "pr??????" { xfail *-*-* } }
+    sink (p);
+  }
+
+  {
+    char *d = p->a8;
+    char *s = p->bn + j;
+    sprintf (d, "%s", s);       // { dg-bogus "-Wrestrict" "pr??????" { xfail *-*-* } }
+    sink (p);
+  }
+
+  {
+    char *d = p->a8 + i;
+    char *s = p->bn + j;
+    sprintf (d, "%s", s);       // { dg-bogus "-Wrestrict" "pr??????" { xfail *-*-* } }
+    sink (p);
+  }
+
+  {
+    char *d = p->an + i;
+    char *s = p->bn;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->an;
+    char *s = p->bn + j;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->an + i;
+    char *s = p->bn + j;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->an + i;
+    char *s = p->a8;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->an;
+    char *s = p->a8 + j;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->an + i;
+    char *s = p->a8 + j;
+    sprintf (d, "%s", s);
+    sink (p);
+  }
+
+  {
+    char *d = p->bn + i;
+    char *s = p->a8;
+    sprintf (d, "%s", s);       // { dg-bogus "-Wrestrict" "pr??????" { xfail *-*-* } }
+    sink (p);
+  }
+
+  {
+    char *d = p->bn;
+    char *s = p->a8 + j;
+    sprintf (d, "%s", s);       // { dg-bogus "-Wrestrict" "pr??????" { xfail *-*-* } }
+    sink (p);
+  }
+
+  {
+    char *d = p->bn + i;
+    char *s = p->a8 + j;
+    sprintf (d, "%s", s);       // { dg-bogus "-Wrestrict" "pr??????" { xfail *-*-* } }
+    sink (p);
+  }
+
+  {
+    char *d = p->bn + i;
+    char *s = p->bn;
+    sprintf (d, "%s", s);       // { dg-warning "may overlap" }
+    sink (p);
+  }
+
+  {
+    char *d = p->bn;
+    char *s = p->bn + j;
+    sprintf (d, "%s", s);       // { dg-warning "may overlap" }
+    sink (p);
+  }
+
+  {
+    char *d = p->bn + i;
+    char *s = p->bn + j;
+    sprintf (d, "%s", s);       // { dg-warning "may overlap" }
+    sink (p);
+  }
+}
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-26.c b/gcc/testsuite/gcc.dg/Wrestrict-26.c
new file mode 100644
index 00000000000..a10c426a081
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wrestrict-26.c
@@ -0,0 +1,114 @@
+/* Verify that sprintf calls with arrays or struct of arrays don't
+   cause -Wrestrict false positives.
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wrestrict -ftrack-macro-expansion=0" } */
+
+#define sprintf(d, f, ...) (sprintf (d, f, __VA_ARGS__), sink (d))
+
+extern void sink (void*, ...);
+extern int (sprintf) (char*, const char*, ...);
+
+extern char ca[][2][8];
+
+void test_array_of_arrays (void)
+{
+  sprintf (ca[0][0], "%s", ca[0][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (ca[0][0], "%s", ca[0][1]);
+  sprintf (ca[0][0], "%s", ca[1][0]);
+  sprintf (ca[0][0], "%s", ca[1][1]);
+
+  sprintf (ca[0][1], "%s", ca[0][0]);
+  sprintf (ca[0][1], "%s", ca[0][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (ca[0][1], "%s", ca[1][0]);
+  sprintf (ca[0][1], "%s", ca[1][1]);
+
+  sprintf (ca[1][0], "%s", ca[0][0]);
+  sprintf (ca[1][0], "%s", ca[0][1]);
+  sprintf (ca[1][0], "%s", ca[1][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (ca[1][0], "%s", ca[1][1]);
+
+  sprintf (ca[1][1], "%s", ca[0][0]);
+  sprintf (ca[1][1], "%s", ca[0][1]);
+  sprintf (ca[1][1], "%s", ca[1][0]);
+  sprintf (ca[1][1], "%s", ca[1][1]);     // { dg-warning "-Wrestrict" }
+}
+
+
+struct A
+{
+  char a[2][2][8];
+  char b[2][2][8];
+  char c[2][2][8];
+};
+
+extern struct A aa[][2];
+
+void test_array_of_structs (void)
+{
+  // Verify that calls with the same elements of the same array trigger
+  // warnings as expected.
+  sprintf (aa[0][0].a[0][0], "%s", aa[0][0].a[0][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][0].a[0][1], "%s", aa[0][0].a[0][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][0].a[1][0], "%s", aa[0][0].a[1][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][0].a[1][1], "%s", aa[0][0].a[1][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][1].a[0][0], "%s", aa[0][1].a[0][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][1].a[0][1], "%s", aa[0][1].a[0][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][1].a[1][0], "%s", aa[0][1].a[1][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[0][1].a[1][1], "%s", aa[0][1].a[1][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][0].a[0][0], "%s", aa[1][0].a[0][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][0].a[0][1], "%s", aa[1][0].a[0][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][0].a[1][0], "%s", aa[1][0].a[1][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][0].a[1][1], "%s", aa[1][0].a[1][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][1].a[0][0], "%s", aa[1][1].a[0][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][1].a[0][1], "%s", aa[1][1].a[0][1]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][1].a[1][0], "%s", aa[1][1].a[1][0]);     // { dg-warning "-Wrestrict" }
+  sprintf (aa[1][1].a[1][1], "%s", aa[1][1].a[1][1]);     // { dg-warning "-Wrestrict" }
+
+#define NOWARN()
+
+  // Exhaustively verify that calls with different elements of the same
+  // array don't cause false positives.
+#undef NOWARN
+#define NOWARN(D, S)				\
+  sprintf (D[0][0], "%s", S[0][0]);		\
+  sprintf (D[0][0], "%s", S[0][1]);		\
+  sprintf (D[0][0], "%s", S[1][0]);		\
+  sprintf (D[0][0], "%s", S[1][1]);		\
+  sprintf (D[0][1], "%s", S[0][0]);		\
+  sprintf (D[0][1], "%s", S[0][1]);		\
+  sprintf (D[0][1], "%s", S[1][0]);		\
+  sprintf (D[0][1], "%s", S[1][1]);		\
+  sprintf (D[1][0], "%s", S[0][0]);		\
+  sprintf (D[1][0], "%s", S[0][1]);		\
+  sprintf (D[1][0], "%s", S[1][0]);		\
+  sprintf (D[1][0], "%s", S[1][1]);		\
+  sprintf (D[1][1], "%s", S[0][0]);		\
+  sprintf (D[1][1], "%s", S[0][1]);		\
+  sprintf (D[1][1], "%s", S[1][0]);		\
+  sprintf (D[1][1], "%s", S[1][1])
+
+  NOWARN (aa[0][0].a, aa[0][1].a);
+  NOWARN (aa[0][0].a, aa[1][0].a);
+  NOWARN (aa[0][0].a, aa[1][1].a);
+
+  NOWARN (aa[0][1].a, aa[0][0].a);
+  NOWARN (aa[0][1].a, aa[1][0].a);
+  NOWARN (aa[0][1].a, aa[1][1].a);
+
+  NOWARN (aa[1][0].a, aa[0][0].a);
+  NOWARN (aa[1][0].a, aa[0][1].a);
+  NOWARN (aa[1][0].a, aa[1][1].a);
+
+#define NOWARN_MEM(M1, M2)			\
+  NOWARN (aa[0][0].M1, aa[0][0].M2);		\
+  NOWARN (aa[0][0].M1, aa[0][1].M2);		\
+  NOWARN (aa[0][0].M1, aa[1][0].M2);		\
+  NOWARN (aa[0][0].M1, aa[1][1].M2)
+
+  NOWARN_MEM (a, b);
+  NOWARN_MEM (a, c);
+  NOWARN_MEM (b, a);
+  NOWARN_MEM (b, c);
+  NOWARN_MEM (c, a);
+  NOWARN_MEM (c, b);
+}

Re: [PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets

[Richard Biener]

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.