tree-optimization/109334: Improve computation for access attribute

  Hi Sid and Jakub,

here is the patch discussed in PR 109334.

Martin



    tree-optimization/109334: Improve computation for access attribute
    
    The fix for PR104970 restricted size computations to the case
    where the access attribute was specified explicitly (no VLA).
    It also restricted it to void pointers or elements with constant
    sizes.  The second restriction is enough to fix the original bug.
    Revert the first change to again allow size computations for VLA
    parameters and for VLA parameters together with an explicit access
    attribute.
    
    gcc/ChangeLog:
    
            PR tree-optimization/109334
            * tree-object-size.cc (parm_object_size): Allow size
            computation for explicit access attributes.
    
    gcc/testsuite/ChangeLog:
    
            PR tree-optimization/109334
            * gcc.dg/builtin-dynamic-object-size-20.c
            (test_parmsz_simple3): Supported again.
            (test_parmsz_external4): New test.
            * gcc.dg/builtin-dynamic-object-size-20.c: New test.
            * gcc.dg/pr104970.c: New test.
  

On 2023-10-26 04:37, Martin Uecker wrote:
> 
> 
> Hi Sid and Jakub,
> 
> here is the patch discussed in PR 109334.
> 

I can't approve, but here's a review:

> Martin
> 
> 
> 
>      tree-optimization/109334: Improve computation for access attribute
>      
>      The fix for PR104970 restricted size computations to the case
>      where the access attribute was specified explicitly (no VLA).
>      It also restricted it to void pointers or elements with constant
>      sizes.  The second restriction is enough to fix the original bug.
>      Revert the first change to again allow size computations for VLA
>      parameters and for VLA parameters together with an explicit access
>      attribute.
>      
>      gcc/ChangeLog:
>      
>              PR tree-optimization/109334
>              * tree-object-size.cc (parm_object_size): Allow size
>              computation for explicit access attributes.
>      
>      gcc/testsuite/ChangeLog:
>      
>              PR tree-optimization/109334
>              * gcc.dg/builtin-dynamic-object-size-20.c
>              (test_parmsz_simple3): Supported again.
>              (test_parmsz_external4): New test.
>              * gcc.dg/builtin-dynamic-object-size-20.c: New test.
>              * gcc.dg/pr104970.c: New test.
> 
> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> index 6da04202ffe..07e3da6f254 100644
> --- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> @@ -455,7 +455,6 @@ test_parmsz_simple2 (size_t sz, char obj[])
>     return __builtin_dynamic_object_size (obj, 0);
>   }
>   
> -/* Implicitly constructed access attributes not supported yet.  */
>   size_t
>   __attribute__ ((noinline))
>   test_parmsz_simple3 (size_t sz, char obj[sz])
> @@ -527,6 +526,13 @@ test_parmsz_internal3 (size_t sz1, size_t sz2, double obj[sz1][sz2])
>     return __builtin_dynamic_object_size (obj, 0);
>   }

This test case now works.  OK.

>   
> +size_t
> +__attribute__ ((noinline))
> +test_parmsz_internal4 (size_t sz1, size_t sz2, double obj[sz1 + 1][4])
> +{
> +  return __builtin_dynamic_object_size (obj, 0);
> +}
> +

New test case that isn't supported yet.  OK.

>   /* Loops.  */
>   
>   size_t
> @@ -721,8 +727,8 @@ main (int argc, char **argv)
>     if (test_parmsz_simple2 (__builtin_strlen (argv[0]) + 1, argv[0])
>         != __builtin_strlen (argv[0]) + 1)
>       FAIL ();
> -  /* Only explicitly added access attributes are supported for now.  */
> -  if (test_parmsz_simple3 (__builtin_strlen (argv[0]) + 1, argv[0]) != -1)
> +  if (test_parmsz_simple3 (__builtin_strlen (argv[0]) + 1, argv[0])
> +      != __builtin_strlen (argv[0]) + 1)
>       FAIL ();
>     int arr[42];
>     if (test_parmsz_scaled (arr, 42) != sizeof (arr))
> @@ -759,6 +765,8 @@ main (int argc, char **argv)
>       FAIL ();
>     if (test_parmsz_internal3 (4, 4, obj) != -1)
>       FAIL ();
> +  if (test_parmsz_internal4 (3, 4, obj) != -1)
> +    FAIL ();
>     if (test_loop (arr, 42, 0, 32, 1) != 10 * sizeof (int))
>       FAIL ();
>     if (test_loop (arr, 42, 32, -1, -1) != 0)
> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c
> new file mode 100644
> index 00000000000..2c8e07dd98d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c
> @@ -0,0 +1,49 @@
> +/* PR 109334
> + * { dg-do run }
> + * { dg-options "-O1" } */
> +
> +
> +[[gnu::noinline,gnu::noipa]]
> +int f(int n, int buf[n])
> +    [[gnu::access(read_only, 2, 1)]]
> +{
> +    return __builtin_dynamic_object_size(buf, 0);
> +}
> +
> +[[gnu::noinline,gnu::noipa]]
> +int g(int n, int buf[])
> +    [[gnu::access(read_only, 2, 1)]]
> +{
> +    return __builtin_dynamic_object_size(buf, 0);
> +}
> +
> +[[gnu::noinline,gnu::noipa]]
> +int h(int n, int buf[n])
> +{
> +    return __builtin_dynamic_object_size(buf, 0);
> +}
> +
> +int dummy(int x) { return x + 1; }
> +
> +[[gnu::noinline,gnu::noipa]]
> +int i(int n, int buf[dummy(n)])
> +{
> +    return __builtin_dynamic_object_size(buf, 0);
> +}
> +
> +int main()
> +{
> +    int n = 10;
> +    int buf[n];
> +    if (n * sizeof(int) != f(n, buf))
> +        __builtin_abort();
> +    if (n * sizeof(int) != g(n, buf))
> +        __builtin_abort();
> +    if (n * sizeof(int) != h(n, buf))
> +        __builtin_abort();
> +
> +    (void)i(n, buf);

f(), g(), h() supported, but i() isn't.  OK.

> +
> +    return 0;
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/pr104970.c b/gcc/testsuite/gcc.dg/pr104970.c
> new file mode 100644
> index 00000000000..e24a7f22dfb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr104970.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -D_FORTIFY_SOURCE=2" } */

The -D_FORTIFY_SOURCE=2 shouldn't be necessary since it doesn't really 
do anything in the context of this test.

> +
> +__inline void
> +memset2(void *__dest, int __ch, long __len) {
> +  long __trans_tmp_1 = __builtin_dynamic_object_size(__dest, 0);
> +  __builtin___memset_chk(__dest, __ch, __len, __trans_tmp_1);
> +}
> +
> +void
> +mleye(int l, double E[][l]) { memset2(E, 0, sizeof(double)); }

New regression test for the ICE reported in pr104970.  OK.

> +
> +
> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> index a62af050056..28f27adf9ca 100644
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -1575,8 +1575,8 @@ parm_object_size (struct object_size_info *osi, tree var)
>     tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm)));
>     tree sz = NULL_TREE;
>   
> -  /* If we have an explicit access attribute with a usable size argument... */
> -  if (access && access->sizarg != UINT_MAX && !access->internal_p
> +  /* If we have an access attribute with a usable size argument... */
> +  if (access && access->sizarg != UINT_MAX

Dropped the unnecessary internal_p test.  OK.

>         /* ... and either PARM is void * or has a type that is complete and has a
>   	 constant size... */
>         && ((typesize && poly_int_tree_p (typesize))
> @@ -1587,10 +1587,14 @@ parm_object_size (struct object_size_info *osi, tree var)
>         unsigned argpos = 0;
>   
>         /* ... then walk through the parameters to pick the size parameter and
> -	 safely scale it by the type size if needed.  */
> +	 safely scale it by the type size if needed.
> +
> +	 TODO: we could also compute the size of VLAs where the size is
> +	 given by a function parameter.  */

Isn't this testcase h() in builtin-dynamic-object-size-20.c?  If you're 
referring to testcase i(), then maybe "where the size is given by a 
non-trivial function of a function parameter, e.g.
fn (size_t n, char buf[dummy(n)])."

>         for (arg = fnargs; arg; arg = TREE_CHAIN (arg), ++argpos)
> -	if (argpos == access->sizarg && INTEGRAL_TYPE_P (TREE_TYPE (arg)))
> +	if (argpos == access->sizarg)
>   	  {
> +	    gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (arg)));
>   	    sz = get_or_create_ssa_default_def (cfun, arg);
>   	    if (sz != NULL_TREE)
>   	      {
> 

We rely on the frontend to make sure that the arg at sizarg is an 
integral type.  OK.

Overall the change looks OK with a few nits I pointed out above.

Thanks,
Sid
  

> Am 26.10.2023 um 13:51 schrieb Siddhesh Poyarekar <siddhesh@gotplt.org>:
> 
> On 2023-10-26 04:37, Martin Uecker wrote:
>> Hi Sid and Jakub,
>> here is the patch discussed in PR 109334.
> 
> I can't approve, but here's a review:

Ok

Thanks for the review,
Richard 

>> Martin
>>     tree-optimization/109334: Improve computation for access attribute
>>          The fix for PR104970 restricted size computations to the case
>>     where the access attribute was specified explicitly (no VLA).
>>     It also restricted it to void pointers or elements with constant
>>     sizes.  The second restriction is enough to fix the original bug.
>>     Revert the first change to again allow size computations for VLA
>>     parameters and for VLA parameters together with an explicit access
>>     attribute.
>>          gcc/ChangeLog:
>>                  PR tree-optimization/109334
>>             * tree-object-size.cc (parm_object_size): Allow size
>>             computation for explicit access attributes.
>>          gcc/testsuite/ChangeLog:
>>                  PR tree-optimization/109334
>>             * gcc.dg/builtin-dynamic-object-size-20.c
>>             (test_parmsz_simple3): Supported again.
>>             (test_parmsz_external4): New test.
>>             * gcc.dg/builtin-dynamic-object-size-20.c: New test.
>>             * gcc.dg/pr104970.c: New test.
>> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
>> index 6da04202ffe..07e3da6f254 100644
>> --- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
>> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
>> @@ -455,7 +455,6 @@ test_parmsz_simple2 (size_t sz, char obj[])
>>    return __builtin_dynamic_object_size (obj, 0);
>>  }
>>  -/* Implicitly constructed access attributes not supported yet.  */
>>  size_t
>>  __attribute__ ((noinline))
>>  test_parmsz_simple3 (size_t sz, char obj[sz])
>> @@ -527,6 +526,13 @@ test_parmsz_internal3 (size_t sz1, size_t sz2, double obj[sz1][sz2])
>>    return __builtin_dynamic_object_size (obj, 0);
>>  }
> 
> This test case now works.  OK.
> 
>>  +size_t
>> +__attribute__ ((noinline))
>> +test_parmsz_internal4 (size_t sz1, size_t sz2, double obj[sz1 + 1][4])
>> +{
>> +  return __builtin_dynamic_object_size (obj, 0);
>> +}
>> +
> 
> New test case that isn't supported yet.  OK.
> 
>>  /* Loops.  */
>>    size_t
>> @@ -721,8 +727,8 @@ main (int argc, char **argv)
>>    if (test_parmsz_simple2 (__builtin_strlen (argv[0]) + 1, argv[0])
>>        != __builtin_strlen (argv[0]) + 1)
>>      FAIL ();
>> -  /* Only explicitly added access attributes are supported for now.  */
>> -  if (test_parmsz_simple3 (__builtin_strlen (argv[0]) + 1, argv[0]) != -1)
>> +  if (test_parmsz_simple3 (__builtin_strlen (argv[0]) + 1, argv[0])
>> +      != __builtin_strlen (argv[0]) + 1)
>>      FAIL ();
>>    int arr[42];
>>    if (test_parmsz_scaled (arr, 42) != sizeof (arr))
>> @@ -759,6 +765,8 @@ main (int argc, char **argv)
>>      FAIL ();
>>    if (test_parmsz_internal3 (4, 4, obj) != -1)
>>      FAIL ();
>> +  if (test_parmsz_internal4 (3, 4, obj) != -1)
>> +    FAIL ();
>>    if (test_loop (arr, 42, 0, 32, 1) != 10 * sizeof (int))
>>      FAIL ();
>>    if (test_loop (arr, 42, 32, -1, -1) != 0)
>> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c
>> new file mode 100644
>> index 00000000000..2c8e07dd98d
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-20.c
>> @@ -0,0 +1,49 @@
>> +/* PR 109334
>> + * { dg-do run }
>> + * { dg-options "-O1" } */
>> +
>> +
>> +[[gnu::noinline,gnu::noipa]]
>> +int f(int n, int buf[n])
>> +    [[gnu::access(read_only, 2, 1)]]
>> +{
>> +    return __builtin_dynamic_object_size(buf, 0);
>> +}
>> +
>> +[[gnu::noinline,gnu::noipa]]
>> +int g(int n, int buf[])
>> +    [[gnu::access(read_only, 2, 1)]]
>> +{
>> +    return __builtin_dynamic_object_size(buf, 0);
>> +}
>> +
>> +[[gnu::noinline,gnu::noipa]]
>> +int h(int n, int buf[n])
>> +{
>> +    return __builtin_dynamic_object_size(buf, 0);
>> +}
>> +
>> +int dummy(int x) { return x + 1; }
>> +
>> +[[gnu::noinline,gnu::noipa]]
>> +int i(int n, int buf[dummy(n)])
>> +{
>> +    return __builtin_dynamic_object_size(buf, 0);
>> +}
>> +
>> +int main()
>> +{
>> +    int n = 10;
>> +    int buf[n];
>> +    if (n * sizeof(int) != f(n, buf))
>> +        __builtin_abort();
>> +    if (n * sizeof(int) != g(n, buf))
>> +        __builtin_abort();
>> +    if (n * sizeof(int) != h(n, buf))
>> +        __builtin_abort();
>> +
>> +    (void)i(n, buf);
> 
> f(), g(), h() supported, but i() isn't.  OK.
> 
>> +
>> +    return 0;
>> +}
>> +
>> diff --git a/gcc/testsuite/gcc.dg/pr104970.c b/gcc/testsuite/gcc.dg/pr104970.c
>> new file mode 100644
>> index 00000000000..e24a7f22dfb
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr104970.c
>> @@ -0,0 +1,13 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O1 -D_FORTIFY_SOURCE=2" } */
> 
> The -D_FORTIFY_SOURCE=2 shouldn't be necessary since it doesn't really do anything in the context of this test.
> 
>> +
>> +__inline void
>> +memset2(void *__dest, int __ch, long __len) {
>> +  long __trans_tmp_1 = __builtin_dynamic_object_size(__dest, 0);
>> +  __builtin___memset_chk(__dest, __ch, __len, __trans_tmp_1);
>> +}
>> +
>> +void
>> +mleye(int l, double E[][l]) { memset2(E, 0, sizeof(double)); }
> 
> New regression test for the ICE reported in pr104970.  OK.
> 
>> +
>> +
>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>> index a62af050056..28f27adf9ca 100644
>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -1575,8 +1575,8 @@ parm_object_size (struct object_size_info *osi, tree var)
>>    tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm)));
>>    tree sz = NULL_TREE;
>>  -  /* If we have an explicit access attribute with a usable size argument... */
>> -  if (access && access->sizarg != UINT_MAX && !access->internal_p
>> +  /* If we have an access attribute with a usable size argument... */
>> +  if (access && access->sizarg != UINT_MAX
> 
> Dropped the unnecessary internal_p test.  OK.
> 
>>        /* ... and either PARM is void * or has a type that is complete and has a
>>       constant size... */
>>        && ((typesize && poly_int_tree_p (typesize))
>> @@ -1587,10 +1587,14 @@ parm_object_size (struct object_size_info *osi, tree var)
>>        unsigned argpos = 0;
>>          /* ... then walk through the parameters to pick the size parameter and
>> -     safely scale it by the type size if needed.  */
>> +     safely scale it by the type size if needed.
>> +
>> +     TODO: we could also compute the size of VLAs where the size is
>> +     given by a function parameter.  */
> 
> Isn't this testcase h() in builtin-dynamic-object-size-20.c?  If you're referring to testcase i(), then maybe "where the size is given by a non-trivial function of a function parameter, e.g.
> fn (size_t n, char buf[dummy(n)])."
> 
>>        for (arg = fnargs; arg; arg = TREE_CHAIN (arg), ++argpos)
>> -    if (argpos == access->sizarg && INTEGRAL_TYPE_P (TREE_TYPE (arg)))
>> +    if (argpos == access->sizarg)
>>        {
>> +        gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (arg)));
>>          sz = get_or_create_ssa_default_def (cfun, arg);
>>          if (sz != NULL_TREE)
>>            {
> 
> We rely on the frontend to make sure that the arg at sizarg is an integral type.  OK.
> 
> Overall the change looks OK with a few nits I pointed out above.
> 
> Thanks,
> Sid
  

Thanks, Sid!

(one comment below)

Am Donnerstag, dem 26.10.2023 um 07:51 -0400 schrieb Siddhesh Poyarekar:
> On 2023-10-26 04:37, Martin Uecker wrote:

> 
> >         /* ... and either PARM is void * or has a type that is complete and has a
> >   	 constant size... */
> >         && ((typesize && poly_int_tree_p (typesize))
> > @@ -1587,10 +1587,14 @@ parm_object_size (struct object_size_info *osi, tree var)
> >         unsigned argpos = 0;
> >   
> >         /* ... then walk through the parameters to pick the size parameter and
> > -	 safely scale it by the type size if needed.  */
> > +	 safely scale it by the type size if needed.
> > +
> > +	 TODO: we could also compute the size of VLAs where the size is
> > +	 given by a function parameter.  */
> 
> Isn't this testcase h() in builtin-dynamic-object-size-20.c?  If you're 
> referring to testcase i(), then maybe "where the size is given by a 
> non-trivial function of a function parameter, e.g.
> fn (size_t n, char buf[dummy(n)])."

h() is supported.  For i() we would need something as
__builtin_access__with_size to record the result of dummy().

But the comment refers to the simpler case:

fn (size_t n, char (*buf)[n])
	[[gnu::access(read_write, 2, 1)]]

This doesn't work because buf[n] does not have constant
size, but it could be made to work more easily because
the size is directly given by a function argument.

Martin


> 
> >         for (arg = fnargs; arg; arg = TREE_CHAIN (arg), ++argpos)
> > -	if (argpos == access->sizarg && INTEGRAL_TYPE_P (TREE_TYPE (arg)))
> > +	if (argpos == access->sizarg)
> >   	  {
> > +	    gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (arg)));
> >   	    sz = get_or_create_ssa_default_def (cfun, arg);
> >   	    if (sz != NULL_TREE)
> >   	      {
> > 
> 
> We rely on the frontend to make sure that the arg at sizarg is an 
> integral type.  OK.
> 
> Overall the change looks OK with a few nits I pointed out above.
> 
> Thanks,
> Sid
  

On 2023-10-28 16:29, Martin Uecker wrote:
>> Isn't this testcase h() in builtin-dynamic-object-size-20.c?  If you're
>> referring to testcase i(), then maybe "where the size is given by a
>> non-trivial function of a function parameter, e.g.
>> fn (size_t n, char buf[dummy(n)])."
> 
> h() is supported.  For i() we would need something as
> __builtin_access__with_size to record the result of dummy().
> 
> But the comment refers to the simpler case:
> 
> fn (size_t n, char (*buf)[n])
> 	[[gnu::access(read_write, 2, 1)]]
> 
> This doesn't work because buf[n] does not have constant
> size, but it could be made to work more easily because
> the size is directly given by a function argument.
> 

Ah, so it would have been nice to have this more detailed explanation in 
the comment for clarity :)

Thanks,
Sid