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

Wed May 3 19:38:12 PDT 2017

For illustrative purposes I've attached a version (a patch on top of your
patch) of some of what I had in mind - there's a few FIXMEs in there where
I stumbled across untested functionality in your patch - so whichever
patch/direction ends up being used, there's some missing test coverage to
address.

One of the big things I'd misunderstood/hadn't expected was that the
visit(DIE) function wasn't recursive. Though the approach I've taken could
be implemented without the recursion (checking the depth of nodes and
reducing the size of the vector as depths are traversed) I think it's
probably a bit easier to read this way.

It seems to simplify things quite a bit, to my mind - and adds a few
missing cases in multiple directions.

It looks like the original patch doesn't catch overlap between two lexical
scopes in the same subprogram at the same level?

Also it looks like the overlapping function code only considers pairs ({1,
2}, {2, 3}, and so on) - so what about if {1, 3} overlap but neither
overlap 2?

(& I also left some code that would catch the case where ranges within a
single range list would overlap - perhaps not wrong, but seems a bit
broken?)

On Wed, May 3, 2017 at 2:17 PM Greg Clayton <clayborg at gmail.com> wrote:

> There should not have been a DW_AT_low_pc on the class type...
>
> On May 3, 2017, at 2:15 PM, Greg Clayton <clayborg at gmail.com> wrote:
>
>
> On May 3, 2017, at 1:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, May 3, 2017 at 1:43 PM Greg Clayton via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> clayborg added a comment.
>>
>> One thing to clarify:
>>
>>   std::vector<RangeInfoType> DieRangeInfos;
>>
>> is a vector that only gets as deep as the DIE depth. It doesn't get every
>> range for every function, block, or inlined function, it just has a stack
>> of ranges for the _current_ DIE depth.
>>
>
> Which is part of what I'm confused by - I wouldn't expect to see/don't
> understand why there's any code that special-cases subprograms here. They
> don't seem special to me.
>
>
> They are special because they are top level functions. DW_TAG_subprogram
> appear at any depth and their parent isn't always a DW_TAG_compile_unit. So
> we might have DWARF like:
>
> 0x0000000b: DW_TAG_compile_unit [1] *
>              DW_AT_low_pc( 0x0000000000001000 )
>              DW_AT_high_pc( 0x0000000000003000 )
>              DW_AT_name( "/tmp/main.c" )
>
> 0x00000020:     DW_TAG_subprogram [2] *
>                  DW_AT_name( "main" )
>                  DW_AT_low_pc( 0x0000000000001000 )
>                  DW_AT_high_pc( 0x0000000000002000 )
>
> 0x00000035:         DW_TAG_lexical_block [3]
>                      DW_AT_low_pc( 0x0000000000001100 )
>                      DW_AT_high_pc( 0x0000000000001200 )
>
> 0x00000046:         NULL
>
> 0x00000047:     DW_TAG_class_type [4] *
>                  DW_AT_low_pc( 0x0000000000001000 )
>                  DW_AT_name( "Foo" )
>
> 0x00000054:         DW_TAG_subprogram [2] *
>                      DW_AT_name( "Bar" )
>                      DW_AT_low_pc( 0x0000000000002000 )
>                      DW_AT_high_pc( 0x0000000000003000 )
>
> 0x00000069:             DW_TAG_lexical_block [3]
>                          DW_AT_low_pc( 0x0000000000002100 )
>                          DW_AT_high_pc( 0x0000000000002200 )
>
> 0x0000007a:             NULL
>
> 0x0000007b:         NULL
>
> 0x0000007c:     NULL
>
> When visiting the DW_TAG_subprogram at 0x00000020 we would do:
>
> Depth = Die.getDepth(); // depth will be 1
> if (!DieRangeInfos[0].Ranges.empty())
>   assert(DieRangeInfos[0].Contains(DieRangeInfos[Depth].Ranges)
>
> since 0x00000020 is at depth 1
>
> Our DieRangeInfos stack would be:
>
> DieRangeInfos[0] = DW_TAG_compile_unit [0x0000000000001000 -
> 0x0000000000003000)
> DieRangeInfos[1] = DW_TAG_subprogram   [0x0000000000001000 -
> 0x0000000000002000)
>
> When visiting the DW_TAG_lexical_block at 0x00000035 we would only check:
>
> assert(DieRangeInfos[Depth-1].Contains(DieRangeInfos[Depth].Ranges))
>
> Our DieRangeInfos stack would be:
>
> DieRangeInfos[0] = DW_TAG_compile_unit [0x0000000000001000 -
> 0x0000000000003000)
> DieRangeInfos[1] = DW_TAG_subprogram   [0x0000000000001000 -
> 0x0000000000002000)
> DieRangeInfos[2]
> = DW_TAG_lexical_block[0x0000000000001100 - 0x0000000000001200)
>
> When visiting 0x00000054 we would again check against the compile units
> ranges:
>
> Depth = Die.getDepth(); // depth will be 2
> if (!DieRangeInfos[0].Ranges.empty())
>   assert(DieRangeInfos[0].Contains(DieRangeInfos[Depth].Ranges)
>
> DieRangeInfos[0] = DW_TAG_compile_unit [0x0000000000001000 -
> 0x0000000000003000)
> DieRangeInfos[1] = DW_TAG_class_type
> DieRangeInfos[2] = DW_TAG_subprogram
> [0x0000000000002000 - 0x0000000000003000)
>
> Then visiting 0x00000069 we do:
>
> assert(DieRangeInfos[Depth-1].Contains(DieRangeInfos[Depth].Ranges))
>
> DieRangeInfos[0] = DW_TAG_compile_unit [0x0000000000001000 -
> 0x0000000000003000)
> DieRangeInfos[1] = DW_TAG_class_type
> DieRangeInfos[2] = DW_TAG_subprogram
> [0x0000000000002000 - 0x0000000000003000)
> DieRangeInfos[3]
> = DW_TAG_lexical_block[0x0000000000002100 - 0x0000000000002200)
>
>
> I'd expect an algorithm that starts at the CU DIE and looks for any DIE
> that has ranges. It wouldn't need special cases for any tag types.
>
>
> You only need to verify your range against something your parent. For
> some, like a DW_TAG_subprogram, it must be in the DW_TAG_compile_unit only
> if it has ranges. For DW_TAG_lexical_block and DW_TAG_inlined_subroutine,
> they MUST be in their parents if their ranges are valid.
>
> Fair point - I was trying to think of if there were any particular scopes
inside a function that could not have ranges, guess not. I could see adding
that as a direct case in my example ("if tag type is
<CU/lexical_block/inlined_subroutine> it must have ranges" - actually,
though, now I reflect on that, that isn't true - abstract origins will have
lexical blocks but they won't have any ranges on them)

>
> It is also possible in other languages to have functions declared inside
> of functions.
>
>
Do you have some concrete examples of this? Would be good to see wat sort
of DWARF existing providers/consumers are using here.

> In that case you want to only verify against your parent and stop at the
> DW_TAG_subprogram level. You could have a stack like:
>
>
> DieRangeInfos[0] = DW_TAG_compile_unit [0x0000000000001000 -
> 0x0000000000003000)
> DieRangeInfos[1] = DW_TAG_subprogram   [0x0000000000001000 -
> 0x0000000000002000)
> DieRangeInfos[2]
> = DW_TAG_lexical_block[0x0000000000001100 - 0x0000000000001200)
> DieRangeInfos[3] = DW_TAG_subprogram   [0x0000000000003000 -
> 0x0000000000004000)
> DieRangeInfos[4]
> = DW_TAG_lexical_block[0x0000000000003100 - 0x0000000000003200)
>
> You would only want to see if:
> - [4] is in [3]
> - [2] is in [1]
> - [3] and [1] are in [0]
>
> Make sense?
>
>
At least the DWARF I've seen for cases like this would have the function
declared locally inside the outer function, but the DWARF for the
definition would be separate/outside. I'd be curious to see a
producer/consumer using this nested idiom. Do you have an example?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170504/fc72a87f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: die_verification.diff
Type: text/x-patch
Size: 13779 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170504/fc72a87f/attachment.bin>