* [PATCH] Strlen pass should set current range query.
@ 2024-05-27 23:24 Andrew MacLeod
2024-05-28 6:49 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Andrew MacLeod @ 2024-05-27 23:24 UTC (permalink / raw)
To: gcc-patches; +Cc: hernandez, aldy
[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]
The strlen pass currently has a local ranger instance, but when it
invokes SCEV or any other shared component, SCEV will not be able to
access to this ranger as it uses get_range_query(). They will be stuck
with global ranges.
Enable/disable ranger should be used instead of a local version which
allows other components to use the current range_query.
Bootstraps on 86_64-pc-linux-gnu, but there is one regression. The
regression is from gcc.dg/Wstringop-overflow-10.c. the function in
question:
void
baz (char *a)
{
char b[16] = "abcdefg";
__builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* { dg-bogus
"specified bound depends on the length of the source argument" } */
}
when compiled with -O2 -Wstringop-overflow -Wstringop-truncation
it now spits out:
b2.c: In function ‘baz’:
b2.c:24:3: warning: ‘__builtin_strncpy’ output 2 truncated before
terminating nul copying <unknown> bytes from a string of the same length
[-Wstringop-truncation]
24 | __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* {
dg-bogus "specified bound depends on the length of the source argument" } */
It seems like maybe something got smarter by setting the current range
query and this is a legitimate warning for this line of code? There
will indeed not be a NULL copied as there are 7 characters in the string...
Is this a testcase issue where this warning should have been issued
before, or am I misunderstanding the warning?
Andrew
PS im afraid of adjusting the status quo in this pass... :-P Not
allowing sSCEV to access the current ranger is causing me other issues
with the fix for 115221. This *should* have been a harmless change
sigh. :-( The whole mechanism should just use the current range-query
instad of passing a ranger pointer aorund. But that a much bigger
issue. one thing at a time.
[-- Attachment #2: 0001-Strlen-pass-should-set-current-range-query.patch --]
[-- Type: text/x-patch, Size: 2250 bytes --]
From 308d5a6b9937c36300e22fcab1ecb67af55dce9e Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Mon, 27 May 2024 13:20:13 -0400
Subject: [PATCH 1/2] Strlen pass should set current range query.
The strlen pass currently has a local ranger instance, but when it
invokes SCEV, scev will not be able to access to this ranger.
Enable/disable ranger shoud be used, allowing other components to use
the current range_query.
* tree-ssa-strlen.cc (strlen_pass::strlen_pass): Add function
pointer and initialize ptr_qry with current range_query.
(strlen_pass::m_ranger): Remove.
(printf_strlen_execute): Enable and disable ranger.
---
gcc/tree-ssa-strlen.cc | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/gcc/tree-ssa-strlen.cc b/gcc/tree-ssa-strlen.cc
index 7596dd80942..c43a2da2836 100644
--- a/gcc/tree-ssa-strlen.cc
+++ b/gcc/tree-ssa-strlen.cc
@@ -235,9 +235,9 @@ get_range (tree val, gimple *stmt, wide_int minmax[2],
class strlen_pass : public dom_walker
{
public:
- strlen_pass (cdi_direction direction)
+ strlen_pass (function *fun, cdi_direction direction)
: dom_walker (direction),
- ptr_qry (&m_ranger),
+ ptr_qry (get_range_query (fun)),
m_cleanup_cfg (false)
{
}
@@ -299,8 +299,6 @@ public:
unsigned HOST_WIDE_INT lenrng[2],
unsigned HOST_WIDE_INT *size, bool *nulterm);
- gimple_ranger m_ranger;
-
/* A pointer_query object to store information about pointers and
their targets in. */
pointer_query ptr_qry;
@@ -5912,9 +5910,10 @@ printf_strlen_execute (function *fun, bool warn_only)
ssa_ver_to_stridx.safe_grow_cleared (num_ssa_names, true);
max_stridx = 1;
+ enable_ranger (fun);
/* String length optimization is implemented as a walk of the dominator
tree and a forward walk of statements within each block. */
- strlen_pass walker (CDI_DOMINATORS);
+ strlen_pass walker (fun, CDI_DOMINATORS);
walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
if (dump_file && (dump_flags & TDF_DETAILS))
@@ -5939,6 +5938,7 @@ printf_strlen_execute (function *fun, bool warn_only)
strlen_to_stridx = NULL;
}
+ disable_ranger (fun);
scev_finalize ();
loop_optimizer_finalize ();
--
2.41.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Strlen pass should set current range query.
2024-05-27 23:24 [PATCH] Strlen pass should set current range query Andrew MacLeod
@ 2024-05-28 6:49 ` Richard Biener
2024-05-28 18:56 ` [COMMITTED] " Andrew MacLeod
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2024-05-28 6:49 UTC (permalink / raw)
To: Andrew MacLeod; +Cc: gcc-patches, hernandez, aldy
On Tue, May 28, 2024 at 1:24 AM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> The strlen pass currently has a local ranger instance, but when it
> invokes SCEV or any other shared component, SCEV will not be able to
> access to this ranger as it uses get_range_query(). They will be stuck
> with global ranges.
>
> Enable/disable ranger should be used instead of a local version which
> allows other components to use the current range_query.
>
> Bootstraps on 86_64-pc-linux-gnu, but there is one regression. The
> regression is from gcc.dg/Wstringop-overflow-10.c. the function in
> question:
>
> void
> baz (char *a)
> {
> char b[16] = "abcdefg";
> __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* { dg-bogus
> "specified bound depends on the length of the source argument" } */
> }
>
> when compiled with -O2 -Wstringop-overflow -Wstringop-truncation
>
> it now spits out:
>
> b2.c: In function ‘baz’:
> b2.c:24:3: warning: ‘__builtin_strncpy’ output 2 truncated before
> terminating nul copying <unknown> bytes from a string of the same length
> [-Wstringop-truncation]
> 24 | __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* {
> dg-bogus "specified bound depends on the length of the source argument" } */
>
> It seems like maybe something got smarter by setting the current range
> query and this is a legitimate warning for this line of code? There
> will indeed not be a NULL copied as there are 7 characters in the string...
>
> Is this a testcase issue where this warning should have been issued
> before, or am I misunderstanding the warning?
I think the warning makes sense in this case. But I'm not sure why the
dg-bogus is there, that looks like a valid complaint as well?!
I think the patch is OK.
Richard.
> Andrew
>
> PS im afraid of adjusting the status quo in this pass... :-P Not
> allowing sSCEV to access the current ranger is causing me other issues
> with the fix for 115221. This *should* have been a harmless change
> sigh. :-( The whole mechanism should just use the current range-query
> instad of passing a ranger pointer aorund. But that a much bigger
> issue. one thing at a time.
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [COMMITTED] Strlen pass should set current range query.
2024-05-28 6:49 ` Richard Biener
@ 2024-05-28 18:56 ` Andrew MacLeod
0 siblings, 0 replies; 3+ messages in thread
From: Andrew MacLeod @ 2024-05-28 18:56 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, hernandez, aldy
[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]
Thanks.
Committed with the change to the testcase.
Bootstraps on x86_64-pc-linux-gnu with no regressions.
Andrew
On 5/28/24 02:49, Richard Biener wrote:
> On Tue, May 28, 2024 at 1:24 AM Andrew MacLeod <amacleod@redhat.com> wrote:
>> The strlen pass currently has a local ranger instance, but when it
>> invokes SCEV or any other shared component, SCEV will not be able to
>> access to this ranger as it uses get_range_query(). They will be stuck
>> with global ranges.
>>
>> Enable/disable ranger should be used instead of a local version which
>> allows other components to use the current range_query.
>>
>> Bootstraps on 86_64-pc-linux-gnu, but there is one regression. The
>> regression is from gcc.dg/Wstringop-overflow-10.c. the function in
>> question:
>>
>> void
>> baz (char *a)
>> {
>> char b[16] = "abcdefg";
>> __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* { dg-bogus
>> "specified bound depends on the length of the source argument" } */
>> }
>>
>> when compiled with -O2 -Wstringop-overflow -Wstringop-truncation
>>
>> it now spits out:
>>
>> b2.c: In function ‘baz’:
>> b2.c:24:3: warning: ‘__builtin_strncpy’ output 2 truncated before
>> terminating nul copying <unknown> bytes from a string of the same length
>> [-Wstringop-truncation]
>> 24 | __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* {
>> dg-bogus "specified bound depends on the length of the source argument" } */
>>
>> It seems like maybe something got smarter by setting the current range
>> query and this is a legitimate warning for this line of code? There
>> will indeed not be a NULL copied as there are 7 characters in the string...
>>
>> Is this a testcase issue where this warning should have been issued
>> before, or am I misunderstanding the warning?
> I think the warning makes sense in this case. But I'm not sure why the
> dg-bogus is there, that looks like a valid complaint as well?!
>
> I think the patch is OK.
>
> Richard.
>
>> Andrew
>>
>> PS im afraid of adjusting the status quo in this pass... :-P Not
>> allowing sSCEV to access the current ranger is causing me other issues
>> with the fix for 115221. This *should* have been a harmless change
>> sigh. :-( The whole mechanism should just use the current range-query
>> instad of passing a ranger pointer aorund. But that a much bigger
>> issue. one thing at a time.
>>
>>
>>
[-- Attachment #2: 0001-Strlen-pass-should-set-current-range-query.patch --]
[-- Type: text/x-patch, Size: 3002 bytes --]
From c43236cb59e11cadda2654edc117d9270dff75c6 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Mon, 27 May 2024 13:20:13 -0400
Subject: [PATCH 1/5] Strlen pass should set current range query.
The strlen pass currently has a local ranger instance, but when it
invokes SCEV, scev will not be able to access to this ranger.
Enable/disable ranger shoud be used, allowing other components to use
the current range_query.
gcc/
* tree-ssa-strlen.cc (strlen_pass::strlen_pass): Add function
pointer and initialize ptr_qry with current range_query.
(strlen_pass::m_ranger): Remove.
(printf_strlen_execute): Enable and disable ranger.
gcc/testsuite/
* gcc.dg/Wstringop-overflow-10.c: Add truncating warning.
---
gcc/testsuite/gcc.dg/Wstringop-overflow-10.c | 2 +-
gcc/tree-ssa-strlen.cc | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c
index bace08ad5d3..ddc27fc0580 100644
--- a/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c
@@ -21,7 +21,7 @@ void
baz (char *a)
{
char b[16] = "abcdefg";
- __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* { dg-bogus "specified bound depends on the length of the source argument" } */
+ __builtin_strncpy (a, b, __builtin_strnlen (b, 7)); /* { dg-warning "output truncated before terminating nul" } */
}
void fill (char *);
diff --git a/gcc/tree-ssa-strlen.cc b/gcc/tree-ssa-strlen.cc
index 7596dd80942..c43a2da2836 100644
--- a/gcc/tree-ssa-strlen.cc
+++ b/gcc/tree-ssa-strlen.cc
@@ -235,9 +235,9 @@ get_range (tree val, gimple *stmt, wide_int minmax[2],
class strlen_pass : public dom_walker
{
public:
- strlen_pass (cdi_direction direction)
+ strlen_pass (function *fun, cdi_direction direction)
: dom_walker (direction),
- ptr_qry (&m_ranger),
+ ptr_qry (get_range_query (fun)),
m_cleanup_cfg (false)
{
}
@@ -299,8 +299,6 @@ public:
unsigned HOST_WIDE_INT lenrng[2],
unsigned HOST_WIDE_INT *size, bool *nulterm);
- gimple_ranger m_ranger;
-
/* A pointer_query object to store information about pointers and
their targets in. */
pointer_query ptr_qry;
@@ -5912,9 +5910,10 @@ printf_strlen_execute (function *fun, bool warn_only)
ssa_ver_to_stridx.safe_grow_cleared (num_ssa_names, true);
max_stridx = 1;
+ enable_ranger (fun);
/* String length optimization is implemented as a walk of the dominator
tree and a forward walk of statements within each block. */
- strlen_pass walker (CDI_DOMINATORS);
+ strlen_pass walker (fun, CDI_DOMINATORS);
walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
if (dump_file && (dump_flags & TDF_DETAILS))
@@ -5939,6 +5938,7 @@ printf_strlen_execute (function *fun, bool warn_only)
strlen_to_stridx = NULL;
}
+ disable_ranger (fun);
scev_finalize ();
loop_optimizer_finalize ();
--
2.41.0
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-28 18:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-27 23:24 [PATCH] Strlen pass should set current range query Andrew MacLeod
2024-05-28 6:49 ` Richard Biener
2024-05-28 18:56 ` [COMMITTED] " Andrew MacLeod
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).