[PATCH] D32821: Add DWARF verifiers to verify address ranges are correct and scoped correctly.

[David Blaikie via llvm-commits]

On Wed, May 3, 2017 at 1:32 PM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> clayborg added a comment.
>
> In https://reviews.llvm.org/D32821#745213, @dblaikie wrote:
>
> > I haven't followed some of the design direction Adrian's been pointing
> this work in - so all of this is subject to discussion, etc:
> >
> > 1. I'd expect not to build an entire representation of all ranges
> up-front, then verify them (uses more memory, etc) - but rather verify
> during a traversal of the DIEs instead
>
>
> The whole reason we need this is to look for overlapping functions between
> all CUs. This helps us to identify exactly which DIEs overlap.


If it's true that no DIEs should overlap, and all DIEs should be a subset
of their parent - then there's no need to compare two DIEs that aren't
siblings* because they can't overlap. The only comparisons that should be
needed would be between immediate siblings, and siblings and parents - not
all possible pairs of nodes. Right?

* for a slightly broad definition of siblings - ignoring any nodes that
don't have ranges (eg: A:R { B { C:R { } } D:R { } } - D and C are siblings
in this sense)


> It is also just a DIE (2 pointers) + vector<pair<uint64_t, uint64_t>. Not
> too costly.


> > 2. for/for loops to check if one range is a subset of another's probably
> rather inefficient. Not sure if LLVM already has better tools for this,
> though I suspect not.
>
> It currently works and I have seen many errors introduced by the compilers
> when they optimize debug info and they get it wrong.


Sure enough - I'm not suggesting the check is a bad idea.


> We only check blocks and inline functions against the containing ranges.


Optimized code can produce /many/ blocks & inline functions, FWIW.


> We can make sure the ranges are coalesced when needed, but other than
> that, what is innefficient about what is there?


RangeInfoType::DoesIntersect is O(N^2) - if the ranges are sorted it should
be O(N), I would think?

& it looks like verifyDebugInfoTag, for example, calls CheckForRangeErrors
against all other nodes at the same level (so, for example - if it was only
looking at subprograms, each subprogram would be compared against all other
subprograms that came before it - even those in other CUs, even when that
other CU's ranges had been proven not to intersect with this CU - similarly
one layer down, all the top level lexical_scopes would be compared against
all other top level lexical scopes, even when the subprogram ranges for the
two subprograms were distinct)


> 99% of the time it is one range seeing if one other range is contained.
>
> >
> >
> > 3. I'm not sure that function ranges need to be special/separate - I'd
> expect any nested ranges (eg: the range of two scopes in a function) to be
> distinct as well, so I'd expect a more general algorithm that walks nodes
> and their children looking for ranges and checking that children are
> subsets of parents, and children are distinct from each other.
>
> Again, not sure what is wrong with the way it is doing it. We just keep a
> stack >= depth of the current DIE with ranges filled in. If a block or
> inlined function is found, it verifies it is in the parent. The DWARF
> doesn't store this information in any useful way we can access, so I
> believe this approach work well.
>

I didn't mean to suggest it won't work - I'm confused by why there is a
separate "verifyDebugInfoOVerlappingFunctionRanges" - I would've expected
that to fall out naturally from the DIE walk range checking.

At the top level, I could imagine since CU DIEs don't have a common parent,
the CU range overlap check might need to be special-cased rather than
falling out naturally from the DIE walk - but only that.


>
>
> https://reviews.llvm.org/D32821
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170503/8147adb5/attachment.html>