* [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings @ 2018-02-20 18:48 Jeff Law 2018-02-21 0:00 ` Joseph Myers 0 siblings, 1 reply; 3+ messages in thread From: Jeff Law @ 2018-02-20 18:48 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings 2018-02-20 18:48 [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings Jeff Law @ 2018-02-21 0:00 ` Joseph Myers 2018-02-21 2:38 ` Jeff Law 0 siblings, 1 reply; 3+ messages in thread From: Joseph Myers @ 2018-02-21 0:00 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings 2018-02-21 0:00 ` Joseph Myers @ 2018-02-21 2:38 ` Jeff Law 0 siblings, 0 replies; 3+ messages in thread From: Jeff Law @ 2018-02-21 2:38 UTC (permalink / raw) To: Joseph Myers; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-21 2:38 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-20 18:48 [PR middle-end/82123] 00/06 Use EVRP range data in sprintf warnings Jeff Law 2018-02-21 0:00 ` Joseph Myers 2018-02-21 2:38 ` Jeff Law
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).