> Am 26.10.2023 um 13:51 schrieb Siddhesh Poyarekar <siddh...@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
