* [PATCH] tree-optimization/109334: Improve computation for access attribute
@ 2023-10-26 8:37 Martin Uecker
2023-10-26 11:51 ` Siddhesh Poyarekar
0 siblings, 1 reply; 5+ messages in thread
From: Martin Uecker @ 2023-10-26 8:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Siddhesh Poyarekar, Jakub Jelinek
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 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);
+
+ 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" } */
+
+__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 && access->sizarg != UINT_MAX && !access->internal_p
+ /* If we have an access attribute with a usable size argument... */
+ if (access && access->sizarg != UINT_MAX
/* ... 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. */
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)
{
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
2023-10-26 8:37 [PATCH] tree-optimization/109334: Improve computation for access attribute Martin Uecker
@ 2023-10-26 11:51 ` Siddhesh Poyarekar
2023-10-26 15:51 ` Richard Biener
2023-10-28 20:29 ` Martin Uecker
0 siblings, 2 replies; 5+ messages in thread
From: Siddhesh Poyarekar @ 2023-10-26 11:51 UTC (permalink / raw)
To: Martin Uecker, gcc-patches; +Cc: Jakub Jelinek
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
2023-10-26 11:51 ` Siddhesh Poyarekar
@ 2023-10-26 15:51 ` Richard Biener
2023-10-28 20:29 ` Martin Uecker
1 sibling, 0 replies; 5+ messages in thread
From: Richard Biener @ 2023-10-26 15:51 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: Martin Uecker, gcc-patches, Jakub Jelinek
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
2023-10-26 11:51 ` Siddhesh Poyarekar
2023-10-26 15:51 ` Richard Biener
@ 2023-10-28 20:29 ` Martin Uecker
2023-10-29 12:15 ` Siddhesh Poyarekar
1 sibling, 1 reply; 5+ messages in thread
From: Martin Uecker @ 2023-10-28 20:29 UTC (permalink / raw)
To: Siddhesh Poyarekar, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tree-optimization/109334: Improve computation for access attribute
2023-10-28 20:29 ` Martin Uecker
@ 2023-10-29 12:15 ` Siddhesh Poyarekar
0 siblings, 0 replies; 5+ messages in thread
From: Siddhesh Poyarekar @ 2023-10-29 12:15 UTC (permalink / raw)
To: Martin Uecker, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-29 12:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-26 8:37 [PATCH] tree-optimization/109334: Improve computation for access attribute Martin Uecker
2023-10-26 11:51 ` Siddhesh Poyarekar
2023-10-26 15:51 ` Richard Biener
2023-10-28 20:29 ` Martin Uecker
2023-10-29 12:15 ` Siddhesh Poyarekar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).