On 6/2/21 6:32 AM, Aldy Hernandez wrote:
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 ??.
Tested on x86-64 Linux.
OK?
If I understand correctly, we will have to ensure that
__builtin_unreachable () and the branches to them are removed by the
end of gimple one way or another. While legacy EVRP is still running,
that isnt a problem.
This patch merely changes the values we export from ranger to be more up
to date... and some fallout you've encountered.
This might get taken care of down the road.. there is some potential for
identifying a range that dominates all uses and making that the global
range.. that would also resolve the issue of builtin_unreachable().
Anyway, we can deal with this when we try to move to ranger-only mode
OK.
