Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
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
Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
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
Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
> Am 26.10.2023 um 13:51 schrieb Siddhesh Poyarekar :
>
> 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 000..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 000..e24a7f22dfb
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr104970.c
>> @@ -0,0 +1,13 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O1
Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
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 000..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 000..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
[PATCH] 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.
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);
}
+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);
+}
+
/* 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 000..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);
+
+return 0;
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr104970.c b/gcc/testsuite/gcc.dg/pr104970.c
new file mode 100644
index 000..e24a7f22dfb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr104970.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -D_FORTIFY_SOURCE=2" } */
+
+__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)); }
+
+
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
