* [PATCH] tree-object-size: Always set computed bit for bdos [PR113012]
@ 2023-12-18 16:42 Siddhesh Poyarekar
2023-12-19 22:57 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Siddhesh Poyarekar @ 2023-12-18 16:42 UTC (permalink / raw)
To: gcc-patches; +Cc: jakub
It is always safe to set the computed bit for dynamic object sizes at
the end of collect_object_sizes_for because even in case of a dependency
loop encountered in nested calls, we have an SSA temporary to actually
finish the object size expression. The reexamine pass for dynamic
object sizes is only for propagation of unknowns and gimplification of
the size expressions, not for loop resolution as in the case of static
object sizes.
gcc/ChangeLog:
PR tree-optimization/113012
* gcc.dg/ubsan/pr113012.c: New test case.
gcc/testsuite/ChangeLog:
PR tree-optimization/113012
* tree-object-size.cc (compute_builtin_object_size): Expand
comment for dynamic object sizes.
(collect_object_sizes_for): Always set COMPUTED bitmap for
dynamic object sizes.
Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
---
Testing:
- Bootstrapped x86_64 and config=ubsan
- Tested i686
OK for trunk and backport to gcc 13 branch?
gcc/testsuite/gcc.dg/ubsan/pr113012.c | 17 +++++++++++++++++
gcc/tree-object-size.cc | 17 ++++++++++++-----
2 files changed, 29 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ubsan/pr113012.c
diff --git a/gcc/testsuite/gcc.dg/ubsan/pr113012.c b/gcc/testsuite/gcc.dg/ubsan/pr113012.c
new file mode 100644
index 00000000000..4fc38cd1171
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/pr113012.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=undefined" } */
+
+int *
+foo (int x, int y, int z, int w)
+{
+ int *p = __builtin_malloc (z * sizeof (int));
+ int *q = p - 1;
+ while (--x > 0)
+ {
+ if (w + 1 > y)
+ q = p - 1;
+ ++*q;
+ ++q;
+ }
+ return p;
+}
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index 28f27adf9ca..434b2fc0bf5 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -1185,10 +1185,12 @@ compute_builtin_object_size (tree ptr, int object_size_type,
osi.tos = NULL;
}
- /* First pass: walk UD chains, compute object sizes that
- can be computed. osi.reexamine bitmap at the end will
- contain what variables were found in dependency cycles
- and therefore need to be reexamined. */
+ /* First pass: walk UD chains, compute object sizes that can be computed.
+ osi.reexamine bitmap at the end will contain what variables that need
+ to be reexamined. For both static and dynamic size computation,
+ reexamination is for propagation across dependency loops. The dynamic
+ case has the additional use case where the computed expression needs
+ to be gimplified. */
osi.pass = 0;
osi.changed = false;
collect_object_sizes_for (&osi, ptr);
@@ -1823,11 +1825,16 @@ collect_object_sizes_for (struct object_size_info *osi, tree var)
gcc_unreachable ();
}
- if (! reexamine || object_sizes_unknown_p (object_size_type, varno))
+ /* Dynamic sizes use placeholder temps to return an answer, so it is always
+ * safe to set COMPUTED for them. */
+ if ((object_size_type & OST_DYNAMIC)
+ || !reexamine || object_sizes_unknown_p (object_size_type, varno))
{
bitmap_set_bit (computed[object_size_type], varno);
if (!(object_size_type & OST_DYNAMIC))
bitmap_clear_bit (osi->reexamine, varno);
+ else if (reexamine)
+ bitmap_set_bit (osi->reexamine, varno);
}
else
{
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tree-object-size: Always set computed bit for bdos [PR113012]
2023-12-18 16:42 [PATCH] tree-object-size: Always set computed bit for bdos [PR113012] Siddhesh Poyarekar
@ 2023-12-19 22:57 ` Jakub Jelinek
2023-12-19 23:23 ` Siddhesh Poyarekar
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2023-12-19 22:57 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: gcc-patches
On Mon, Dec 18, 2023 at 11:42:52AM -0500, Siddhesh Poyarekar wrote:
> It is always safe to set the computed bit for dynamic object sizes at
> the end of collect_object_sizes_for because even in case of a dependency
> loop encountered in nested calls, we have an SSA temporary to actually
> finish the object size expression. The reexamine pass for dynamic
> object sizes is only for propagation of unknowns and gimplification of
> the size expressions, not for loop resolution as in the case of static
> object sizes.
>
> gcc/ChangeLog:
>
> PR tree-optimization/113012
> * gcc.dg/ubsan/pr113012.c: New test case.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/113012
> * tree-object-size.cc (compute_builtin_object_size): Expand
> comment for dynamic object sizes.
> (collect_object_sizes_for): Always set COMPUTED bitmap for
> dynamic object sizes.
You have the gcc/ChangeLog and gcc/testsuite/ChangeLog hunks swapped,
I think this wouldn't get through pre-commit hook.
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -1185,10 +1185,12 @@ compute_builtin_object_size (tree ptr, int object_size_type,
> osi.tos = NULL;
> }
>
> - /* First pass: walk UD chains, compute object sizes that
> - can be computed. osi.reexamine bitmap at the end will
> - contain what variables were found in dependency cycles
> - and therefore need to be reexamined. */
> + /* First pass: walk UD chains, compute object sizes that can be computed.
> + osi.reexamine bitmap at the end will contain what variables that need
This wording is weird.
Perhaps ... will contain versions of SSA_NAMEs that need
to be reexamined. ?
Because varno seems to be SSA_NAME_VERSION.
> @@ -1823,11 +1825,16 @@ collect_object_sizes_for (struct object_size_info *osi, tree var)
> gcc_unreachable ();
> }
>
> - if (! reexamine || object_sizes_unknown_p (object_size_type, varno))
> + /* Dynamic sizes use placeholder temps to return an answer, so it is always
> + * safe to set COMPUTED for them. */
We don't use this style of comments, please replace the * at the start of
second line with a space.
> + if ((object_size_type & OST_DYNAMIC)
> + || !reexamine || object_sizes_unknown_p (object_size_type, varno))
> {
> bitmap_set_bit (computed[object_size_type], varno);
> if (!(object_size_type & OST_DYNAMIC))
> bitmap_clear_bit (osi->reexamine, varno);
> + else if (reexamine)
> + bitmap_set_bit (osi->reexamine, varno);
> }
> else
> {
Otherwise LGTM, but please wait at least a few weeks before backporting to
13.
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tree-object-size: Always set computed bit for bdos [PR113012]
2023-12-19 22:57 ` Jakub Jelinek
@ 2023-12-19 23:23 ` Siddhesh Poyarekar
0 siblings, 0 replies; 3+ messages in thread
From: Siddhesh Poyarekar @ 2023-12-19 23:23 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 2023-12-19 17:57, Jakub Jelinek wrote:
> On Mon, Dec 18, 2023 at 11:42:52AM -0500, Siddhesh Poyarekar wrote:
>> It is always safe to set the computed bit for dynamic object sizes at
>> the end of collect_object_sizes_for because even in case of a dependency
>> loop encountered in nested calls, we have an SSA temporary to actually
>> finish the object size expression. The reexamine pass for dynamic
>> object sizes is only for propagation of unknowns and gimplification of
>> the size expressions, not for loop resolution as in the case of static
>> object sizes.
>>
>> gcc/ChangeLog:
>>
>> PR tree-optimization/113012
>> * gcc.dg/ubsan/pr113012.c: New test case.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR tree-optimization/113012
>> * tree-object-size.cc (compute_builtin_object_size): Expand
>> comment for dynamic object sizes.
>> (collect_object_sizes_for): Always set COMPUTED bitmap for
>> dynamic object sizes.
>
> You have the gcc/ChangeLog and gcc/testsuite/ChangeLog hunks swapped,
> I think this wouldn't get through pre-commit hook.
Oops, fixed.
>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -1185,10 +1185,12 @@ compute_builtin_object_size (tree ptr, int object_size_type,
>> osi.tos = NULL;
>> }
>>
>> - /* First pass: walk UD chains, compute object sizes that
>> - can be computed. osi.reexamine bitmap at the end will
>> - contain what variables were found in dependency cycles
>> - and therefore need to be reexamined. */
>> + /* First pass: walk UD chains, compute object sizes that can be computed.
>> + osi.reexamine bitmap at the end will contain what variables that need
>
> This wording is weird.
> Perhaps ... will contain versions of SSA_NAMEs that need
> to be reexamined. ?
> Because varno seems to be SSA_NAME_VERSION.
Ack, it's unrelated to my change, but fixed since I touched the comment
block.
>> @@ -1823,11 +1825,16 @@ collect_object_sizes_for (struct object_size_info *osi, tree var)
>> gcc_unreachable ();
>> }
>>
>> - if (! reexamine || object_sizes_unknown_p (object_size_type, varno))
>> + /* Dynamic sizes use placeholder temps to return an answer, so it is always
>> + * safe to set COMPUTED for them. */
>
> We don't use this style of comments, please replace the * at the start of
> second line with a space.
Oops, fixed.
>> + if ((object_size_type & OST_DYNAMIC)
>> + || !reexamine || object_sizes_unknown_p (object_size_type, varno))
>> {
>> bitmap_set_bit (computed[object_size_type], varno);
>> if (!(object_size_type & OST_DYNAMIC))
>> bitmap_clear_bit (osi->reexamine, varno);
>> + else if (reexamine)
>> + bitmap_set_bit (osi->reexamine, varno);
>> }
>> else
>> {
>
> Otherwise LGTM, but please wait at least a few weeks before backporting to
> 13.
OK, I'll push with fixes to trunk in a bit and then push to 13 next year.
Thanks,
Sid
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-19 23:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-18 16:42 [PATCH] tree-object-size: Always set computed bit for bdos [PR113012] Siddhesh Poyarekar
2023-12-19 22:57 ` Jakub Jelinek
2023-12-19 23:23 ` 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).