diff mbox series

[COMMITTED] Strlen pass should set current range query.

Message ID 72e08498-033e-4492-b6d2-f1687bb34b22@redhat.com
State New
Headers show
Series [COMMITTED] Strlen pass should set current range query. | expand

Commit Message

Andrew MacLeod May 28, 2024, 6:56 p.m. UTC 
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.
>>
>>
>>
diff mbox series

Patch

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