On Wed, Oct 25, 2017 at 5:44 PM, Jeff Law <l...@redhat.com> wrote: > On 10/24/2017 11:35 AM, Martin Sebor wrote: >> On 10/23/2017 05:14 PM, Jeff Law wrote: >>> >>> Martin, >>> >>> I'd like your thoughts on this patch. >>> >>> One of the things I'm working on is changes that would allow passes that >>> use dominator walks to trivially perform context sensitive range >>> analysis as a part of their dominator walk. >>> >>> As I outlined earlier this would allow us to easily fix the false >>> positive sprintf warning reported a week or two ago. >>> >>> This patch converts the sprintf warning code to perform a dominator walk >>> rather than just walking the blocks in whatever order they appear in the >>> basic block array. >>> >>> From an implementation standpoint we derive a new class sprintf_dom_walk >>> from the dom_walker class. Like other dom walkers we walk statements >>> from within the before_dom_children member function. Very standard >>> stuff. >>> >>> I moved handle_gimple_call and various dependencies into the >>> sprintf_dom_walker class to facilitate calling handle_gimple_call from >>> within the before_dom_children member function. There's light fallout >>> in various places where the call_info structure was explicitly expected >>> to be found in the pass_sprintf_length class, but is now found in the >>> sprintf_dom_walker class. >>> >>> This has been bootstrapped and regression tested on x86_64-linux-gnu. >>> I've also layered my embedded VRP analysis on top of this work and >>> verified that it does indeed fix the reported false positive. >>> >>> Thoughts? >> >> If it lets us improve the quality of the range information I can't >> think of a downside. > It's potentially slower simply because the domwalk interface is more > expensive than just iterating over the blocks with FOR_EACH_BB. But > that's about it. I think the ability to get more accurate range > information will make the compile-time hit worth it. > >> >> Besides the sprintf pass, a number of other areas depend on ranges, >> most of all the -Wstringop-overflow and truncation warnings and >> now -Wrestrict (once my enhancement is approved). It would be nice >> to be able to get the same improvements there. Does it mean that >> those warnings will need to be moved into a standalone pass? (I'm >> not opposed to it, just wondering what to expect if this is >> the route we want to go.) > They don't necessarily have to be a standalone pass -- they just have to > be implementable as part of a dominator walk to get the cheap context > sensitive range data. > > So IIRC you've got some code to add additional warnings within the > strlen pass. That pass is already a dominator walk. In theory you'll > just add a member to the strlen_dom_walker class, then a call in > before_dom_children and after_dom_children virtuals and you should be > able to query the context sensitive range information. > > For warnings that occur in code that is not easily structured as a > dominator walk, Andrew's work will definitely be a better choice. > > Andrew's work will almost certainly also generate even finer grained > ranges because it can work on an arbitrary path through the CFG rather > than relying on dominance relationships. Consider > > A > / \ > B C > \ / > D > > Range information implied by the edge A->B is usable within B because > the edge A->B dominates B. Similarly for range information implied by > A->C being available in C. But range information implied by A->B is not > available in D because A->B does not dominate D. SImilarly range > information implied by A->C is not available in D. > > I touched on this in a private message recently. Namely that exploiting > range data in non-dominated blocks feels a *lot* like jump threading and > should likely be structured as a backwards walk query (and thus is more > suitable for Andrew's infrastructure).
On the contrary - with a backward walk you don't know which way to go. >From D to B or to C? With a forward walk there's no such ambiguity (unless you start from A). Note I have patches for EVRP merging ranges from B and C to make the info available for D but usually there's nothing to recover here that isn't also valid in A. Just ranges derived from non-conditional stmts (by means of exploiting undefined behavior) can help here. Richard. > > jeff
