We've been having "issues" in our branch when exporting to the global
space ranges that take into account previously known ranges
(SSA_NAME_RANGE_INFO, etc). For the longest time we had the export
feature turned off because it had the potential of removing
__builtin_unreachable code early in the pipeline. This was causing one
or two tests to fail.
I finally got fed up, and investigated why.
Take the following code:
i_4 = somerandom ();
if (i_4 < 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
<bb 3> :
__builtin_unreachable ();
<bb 4> :
It turns out that both legacy evrp and VRP have code that notices the
above pattern and sets the *global* range for i_4 to [0,MAX]. That is,
the range for i_4 is set, not at BB4, but at the definition site. See
uses of assert_unreachable_fallthru_edge_p() for details.
This global range causes subsequent passes (VRP1 in the testcase below),
to remove the checks and the __builtin_unreachable code altogether.
// pr80776-1.c
int somerandom (void);
void
Foo (void)
{
int i = somerandom ();
if (! (0 <= i))
__builtin_unreachable ();
if (! (0 <= i && i <= 999999))
__builtin_unreachable ();
sprintf (number, "%d", i);
}
This means that by the time the -Wformat-overflow warning runs, the
above sprintf has been left unguarded, and a bogus warning is issued.
Currently the above test does not warn, but that's because of an
oversight in export_global_ranges(). This function is disregarding
known global ranges (SSA_NAME_RANGE_INFO and SSA_NAME_PTR_INFO) and only
setting ranges the ranger knows about.
For the above test the IL is:
<bb 2> :
i_4 = somerandom ();
if (i_4 < 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
<bb 3> :
__builtin_unreachable ();
<bb 4> :
i.0_1 = (unsigned int) i_4;
if (i.0_1 > 999999)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
<bb 5> :
__builtin_unreachable ();
<bb 6> :
_7 = __builtin___sprintf_chk (&number, 1, 7, "%d", i_4);
Legacy evrp has determined that the range for i_4 is [0,MAX] per my
analysis above, but ranger has no known range for i_4 at the definition
site. So at export_global_ranges time, ranger leaves the [0,MAX] alone.
OTOH, evrp sets the global range at the definition for i.0_1 to
[0,999999] per the same unreachable feature. However, ranger has
correctly determined that the range for i.0_1 at the definition is
[0,MAX], which it then proceeds to export. Since the current
export_global_ranges (mistakenly) does not take into account previous
global ranges, the ranges in the global tables end up like this:
i_4: [0, MAX]
i.0_1: [0, MAX]
This causes the first unreachable block to be removed in VRP1, but the
second one to remain. Later VRP can determine that i_4 in the sprintf
call is [0,999999], and no warning is issued.
But... the missing bogus warning is due to current export_global_ranges
ignoring SSA_NAME_RANGE_INFO and friends, something which I'd like to
fix. However, fixing this, gets us back to:
i_4: [0, MAX]
i.0_1: [0, 999999]
Which means, we'll be back to removing the unreachable blocks and
issuing a warning in pr80776-1.c (like we have been since the beginning
of time).
The attached patch fixes export_global_ranges to the expected behavior,
and adds the previous XFAIL to pr80776-1.c, while documenting why this
warning is issued in the first place.
Once legacy evrp is removed, this won't be an issue, as ranges in the IL
will tell the truth. However, this will mean that we will no longer
remove the first __builtin_unreachable combo. But ISTM, that would be
correct behavior ??.
BTW, in addition to this patch we could explore removing the
assert_unreachable_fallthru_edge_p() use in the evrp_analyzer, since it
is no longer needed to get the warnings in the testcases in the original
PR correctly (gcc.dg/pr80776-[12].c).
Tested on x86-64 Linux.
OK?
Aldy
>From 36684dde843a4c9556b97bf030cabef8b9430aa4 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <al...@redhat.com>
Date: Tue, 1 Jun 2021 17:48:30 +0200
Subject: [PATCH 2/2] Use known global ranges in export_global_ranges
This patch modifies export_global_ranges to take into account current
global ranges. It also handles enhances said function to export pointer
global ranges as well.
gcc/ChangeLog:
* gimple-range.cc (gimple_ranger::export_global_ranges): Call
update_global_range.
* value-query.cc (update_global_range): New.
* value-query.h (update_global_range): New.
gcc/testsuite/ChangeLog:
* gcc.dg/pr80776-1.c: XFAIL and document the reason why.
---
gcc/gimple-range.cc | 26 ++++++++-------------
gcc/testsuite/gcc.dg/pr80776-1.c | 12 +++++++++-
gcc/value-query.cc | 39 ++++++++++++++++++++++++++++++++
gcc/value-query.h | 1 +
4 files changed, 61 insertions(+), 17 deletions(-)
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index ed0a0c9702b..af426207092 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -1115,7 +1115,7 @@ gimple_ranger::range_of_stmt (irange &r, gimple *s, tree name)
}
// This routine will export whatever global ranges are known to GCC
-// SSA_RANGE_NAME_INFO fields.
+// SSA_RANGE_NAME_INFO and SSA_NAME_PTR_INFO fields.
void
gimple_ranger::export_global_ranges ()
@@ -1136,24 +1136,18 @@ gimple_ranger::export_global_ranges ()
&& m_cache.get_global_range (r, name)
&& !r.varying_p())
{
- // Make sure the new range is a subset of the old range.
- int_range_max old_range;
- old_range = gimple_range_global (name);
- old_range.intersect (r);
- /* Disable this while we fix tree-ssa/pr61743-2.c. */
- //gcc_checking_assert (old_range == r);
-
- // WTF? Can't write non-null pointer ranges?? stupid set_range_info!
- if (!POINTER_TYPE_P (TREE_TYPE (name)) && !r.undefined_p ())
+ bool updated = update_global_range (r, name);
+
+ if (updated && dump_file)
{
value_range vr = r;
- set_range_info (name, vr);
- if (dump_file)
+ print_generic_expr (dump_file, name , TDF_SLIM);
+ fprintf (dump_file, " --> ");
+ vr.dump (dump_file);
+ fprintf (dump_file, "\n");
+ int_range_max same = vr;
+ if (same != r)
{
- print_generic_expr (dump_file, name , TDF_SLIM);
- fprintf (dump_file, " --> ");
- vr.dump (dump_file);
- fprintf (dump_file, "\n");
fprintf (dump_file, " irange : ");
r.dump (dump_file);
fprintf (dump_file, "\n");
diff --git a/gcc/testsuite/gcc.dg/pr80776-1.c b/gcc/testsuite/gcc.dg/pr80776-1.c
index f3a120b6744..eca5e805ae2 100644
--- a/gcc/testsuite/gcc.dg/pr80776-1.c
+++ b/gcc/testsuite/gcc.dg/pr80776-1.c
@@ -17,5 +17,15 @@ Foo (void)
__builtin_unreachable ();
if (! (0 <= i && i <= 999999))
__builtin_unreachable ();
- sprintf (number, "%d", i); /* { dg-bogus "writing" "" } */
+
+ /* Legacy evrp sets the range of i to [0, MAX] *before* the first conditional,
+ and to [0,999999] *before* the second conditional. This is because both
+ evrp and VRP use trickery to set global ranges when this particular use of
+ a __builtin_unreachable is in play (see uses of
+ assert_unreachable_fallthru_edge_p).
+
+ Setting these ranges at the definition site, causes VRP to remove the
+ unreachable code altogether, leaving the following sprintf unguarded. This
+ causes the bogus warning below. */
+ sprintf (number, "%d", i); /* { dg-bogus "writing" "" { xfail *-*-* } } */
}
diff --git a/gcc/value-query.cc b/gcc/value-query.cc
index f8b457d362c..070d706166e 100644
--- a/gcc/value-query.cc
+++ b/gcc/value-query.cc
@@ -224,6 +224,45 @@ get_ssa_name_ptr_info_nonnull (const_tree name)
return !pi->pt.null;
}
+// Update the global range for NAME into the SSA_RANGE_NAME_INFO and
+// SSA_NAME_PTR_INFO fields. Return TRUE if the range for NAME was
+// updated.
+
+bool
+update_global_range (irange &r, tree name)
+{
+ tree type = TREE_TYPE (name);
+
+ if (r.undefined_p () || r.varying_p ())
+ return false;
+
+ if (INTEGRAL_TYPE_P (type))
+ {
+ // If a global range already exists, incorporate it.
+ if (SSA_NAME_RANGE_INFO (name))
+ {
+ value_range glob;
+ get_ssa_name_range_info (glob, name);
+ r.intersect (glob);
+ }
+ if (r.undefined_p ())
+ return false;
+
+ value_range vr = r;
+ set_range_info (name, vr);
+ return true;
+ }
+ else if (POINTER_TYPE_P (type))
+ {
+ if (r.nonzero_p ())
+ {
+ set_ptr_nonnull (name);
+ return true;
+ }
+ }
+ return false;
+}
+
// Return the legacy global range for NAME if it has one, otherwise
// return VARYING.
diff --git a/gcc/value-query.h b/gcc/value-query.h
index 97da6637747..d0512e40c5a 100644
--- a/gcc/value-query.h
+++ b/gcc/value-query.h
@@ -115,5 +115,6 @@ public:
extern global_range_query global_ranges;
extern value_range gimple_range_global (tree name);
+extern bool update_global_range (irange &r, tree name);
#endif // GCC_QUERY_H
--
2.31.1
