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).
jeff
