[PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings

[PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings

[Jeff Law]

So I'm finally getting back to this.

To recap, the issue here is the sprintf pass is querying global range
information.  As a result the range for the key object is not narrowed
by the conditionals leading to the sprintf call and we get a false
positive (pr81592 and pr82123).

The plan for the last few months was to use the embeddable range
analyzer to get the narrowed range.  I'd dropped in a bit of
infrastructure to do that a while back, but got side tracked by spectre
and meltdown before I could push it to completion.

This patch completes the work.  It ties the range analyzer into the
dominator walk (which was trivial) and arranges to query the range
analyzer rather than the global data.

The patch is bigger than one might expect primarily because the points
where we want to issue the queries are in free functions rather than in
member functions.

Rather than go through a round of refactoring to bring those free
functions into the class hierarchy, I just passed around the range data.
 It's the same trivial change in a bunch of places to pass vr_values
down.  So it looks big, but is dead simple in reality.

We query the data in 3 places in the fairly obvious way.  My initial
patch actually created a get_range_info member function within vr_values
that had the same signature as the free function get_range_info  in
tree-ssanames.c

That works, but ultimately I decided it was more confusing than using
the existing get_value_range member function.  So this version uses the
existing get_value_range member function.

The fix is broken into a half-dozen patches in my local tree.  I didn't
see any value in squashing them together -- so I'm posting them as a 6
series kit, even though they're pretty simple.

Bootstrapped and regression tested on x86_64-linux-gnu.  Verified it
fixes both 81592 and 82123.

Jeff

Re: [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings

[Joseph Myers]

Does this help with any of the cases in bug 80776 that weren't already 
fixed, or are those distinct despite looking similar?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings

[Jeff Law]

On 02/20/2018 05:00 PM, Joseph Myers wrote:
> Does this help with any of the cases in bug 80776 that weren't already 
> fixed, or are those distinct despite looking similar?
> 
I don't think so.

THe __builtin_unreachable markers are removed by vrp1 -- well before the
sprintf warning code gets run.

So the sprintf warning code never gets to exploit the properties implied
by the __builtin_unreachable calls.


It doesn't look like VRP records the narrowed ranges implied by the
__builtin_unreachable calls.

After ASSERT_EXPR insertion we have:

;;   basic block 6, loop depth 0, count 1072883002 (estimated locally),
maybe hot
;;    prev block 5, next block 1, flags: (NEW, REACHABLE, VISITED)
;;    pred:       4 [100.0% (guessed)]  count:1072883003 (estimated
locally) (FALSE_VALUE,EXECUTABLE)
  i_7 = ASSERT_EXPR <i_6, (unsigned int) i_6 <= 999999>;
  __builtin___sprintf_chk (&number, 1, 7, "%d", i_7);
  return;

ANd the ranges computed by VRP:


i.0_1: [0, 999999]
i_4: [0, +INF]
i_6: [0, +INF]  EQUIVALENCES: { i_4 } (1 elements)
i_7: [0, 999999]  EQUIVALENCES: { i_4 i_6 } (2 elements)

So VRP does identify the narrow range for i_7.

But then we remove the ASSERT_EXPRs and we're left with:


  i_4 = somerandom ();
  i.0_1 = (unsigned int) i_4;
  __builtin___sprintf_chk (&number, 1, 7, "%d", i_4);
  return;

Subsequent EVRP analysis will start with the range of i_4 as a seed.
BUt there's nothing to further narrow that range.

If ASSERT_EXPR removal could be taught to use i_7 I suspect the right
things would "just happen".  I haven't thought at all about what might
be required to have VRP do-the-right-thing.  Given the overall desire to
drop ASSERT_EXPRs and the range propagation step in VRP in favor of EVRP
style analysis I doubt anyone is likely to spend much time on fixing
this in the old style VRP analysis.

jeff