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

[David Blaikie via llvm-commits]

On Mon, May 8, 2017 at 4:19 PM Greg Clayton <clayborg at gmail.com> wrote:

> On May 8, 2017, at 4:03 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, May 3, 2017 at 1:57 PM Greg Clayton <clayborg at gmail.com> wrote:
>
>> On May 3, 2017, at 1:45 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> 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?
>>
>>
>> That is how this is coded. Only top DW_TAG_subprogram DIEs get added to
>> the AllFunctionRangeInfos. So when checking for overlap, we just check
>> between adjacent DW_TAG_subprogram. Since these are in a std::set, they are
>> ordered already. So we just check:
>>
>> for I in AllFunctionRangeInfos.size():
>>   AllFunctionRangeInfos[I].Contains(AllFunctionRangeInfos[I+1])
>>
>
> Ah, that makes loads of sense - nice!
>
> So why aren't the lexical blocks scopes handled the same way?
>
>
> They are now. The new NonOverlappingRanges class does this by
> storing things a std::set. We now check everything on the fly.
>
>
> (& indeed, this is what I'm getting at about the duplicate code across
> this - I'm not understanding why there are two different ways of testing
> intersects or subset - and two different codepaths for the error messages
> too)
>
> What I'm picturing is a single device, much like what I wrote in the patch
> I provided last week, I think. That device/class represents a scope (in the
> general sense, not the lexical_scope sense), with an optional parent node +
> range and a list of children node+ranges. There would be an explicit
> instance for the file level (with no parent) that is used by every
> compile_unit's ranges. Another (hopefully local/non-member version, passed
> down - but it could be a member) created when visiting a top level compile
> unit - for its subprograms. And any number more created on the stack for
> the lexical scopes.
>
>
> That is essentially what we have now, we just have a const class and one
> in/out class.
>
>
> This object would handle error message printing for both sibling overlap
> and not-contained-in-parent errors, in one place with one
> message/handling/etc.
>
>
> I guess we could switch over to using NonOverlappingRanges in
> the DieRangeInfo as well. I didn't want to put the error handling inside
> these classes because they need to access the error count.
>

Rolling the error handling in would reduce a bunch of the
duplication/complexity, I think.

Worst case, the functions that could error could return the number of
errors they printed, and be used like:

  ErrorCount += NOR.AddRange(Die);

For example.

This is pretty close to what is there now, I just don't use
NonOverlappingRanges
> in the DieRangeInfo currently... I will switch that over and see what
> simplifies out.
>
> Greg
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170508/a9967d8d/attachment.html>