This is an archive of the discontinued LLVM Phabricator instance.

Add iterator support to DWARFDie to allow child DIE iteration.
ClosedPublic

Authored by clayborg on Jan 4 2017, 10:31 AM.

Diff Detail

Event Timeline

clayborg updated this revision to Diff 83080.Jan 4 2017, 10:31 AM
clayborg retitled this revision from to Add iterator support to DWARFDie to allow child DIE iteration..
clayborg updated this object.
dblaikie added inline comments.Jan 4 2017, 11:22 AM
include/llvm/DebugInfo/DWARF/DWARFDie.h
46

Probably best to define != in terms of ==, or the other way around:

return !(*this == RHS);

(also, I generally suggest that any op overload that can be a non-member should be a non-member: http://stackoverflow.com/questions/4421706/operator-overloading/4421729#4421729 )

& is this clang-formatted? (I'm surprised by the space after the operator keyword)

375
iterator() = default;
376

If DWARFDie is cheap enough to copy into a member of an iterator it's probably cheap enough to pass by value here.

383–384

Does the facade provide a way to only have to implement one of these two?

lib/DebugInfo/DWARF/DWARFDie.cpp
448–450

This could probably be inline - begin/end wouldn't be bad as inline definitions either.

tools/dsymutil/DwarfLinker.cpp
1800–1801

When can a child be null? Oh, we're still leaving null as a child in the iteration. That seems really weird.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1131–1138

This seems like more testing than I'd expect, given the code change.

If you want to exercise the same cases for iterator accessors as for the other accessors (could we remove the other accessors (getChild/getSibling) & just rely on iteration instead?) - then perhaps a parameterized test.

Otherwise I'd expect only some fairly brief testing that iterators appear to work - no need to redundantly test the cases that are interesting for getChild/getSibling.

1199–1204

Can you create a NULL DWARFDie object directly & test that separately (in a separate test function)?

1202–1203

can use x->y rather than (*x).y, hopefully

1206–1212

Split this out as a separate test function

clayborg added inline comments.Jan 4 2017, 11:43 AM
lib/DebugInfo/DWARF/DWARFDie.cpp
450

I can inline children(), but not begin() and end() since they use the iterator class which must be forward declared since it has a member variable:

DWARFDie Die;
tools/dsymutil/DwarfLinker.cpp
1800–1801

Yeah, we currently want it there for dumping, but not for real world use. Should we add a "bool IncludeNull" as a parameter to the children function to allow clients to choose?

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1138

ok, I can simplify and remove B1, B2, C, C1 and D.

1202–1203

I currently return a DWARFDie instance from the operator * for DWARFDie::iterator as I didn't want clients to modify the DWARFDie in the iterator by returning it by reference since it is what controls the iteration. This makes the -> operator not work as it takes the address of the returned DWARFDie and causes a compile error. Should we just have "DWARFDie::iterator operator*" return a reference to the contained item? I didn't like either approach.

1204

No, you can't get a NULL die without creating DWARF for it, so this should probably stay here.

1212

Why? this is testing the iterators and their functionality.

clayborg updated this revision to Diff 83103.Jan 4 2017, 12:10 PM

Addressed inline comments.

clayborg marked 7 inline comments as done.Jan 4 2017, 12:12 PM

Marked things as done.

clayborg updated this revision to Diff 83267.Jan 5 2017, 9:42 AM
  • Inlined DWARFDie::iterator::begin(), DWARFDie::iterator::end() and DWARFDie::iterator::children()
  • Don't include NULL DIE when using DWARFDie::iterator as most clients want this behavior
  • Simplify tests for iterators to bare minimum
dblaikie added inline comments.Jan 5 2017, 10:17 AM
include/llvm/DebugInfo/DWARF/DWARFDie.h
389–391

I think this inconsistency is going to be confusing to users (I suspect the right thing is to only provide the iterator and don't provide the sibling API to users at all - and have the dumpers reconstitute the NULL dies for their own needs)

389–391

I'm guessing this will have problems with a present yet zero-length child list? (in that case the children() iteration would include the null die, since this check isn't done on construction, only on increment)

397

If you have op* return a reference (as it's meant to to conform to the iterator concept in the C++ standard) do you still need to write op->, or do you get that for free from the facade?

410–412

Probably just put this one directly inline in the class, since it can be

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1164–1170

Testing that they're equal should be sufficient - it's certainly not necessary to define or test what happens when an end iterator is deferenced.

clayborg updated this revision to Diff 83272.Jan 5 2017, 10:45 AM

Fixed DWARFDie::iterator to skip NULL DIE if it is initialized with one.

clayborg marked an inline comment as done.Jan 5 2017, 10:46 AM
clayborg added inline comments.
include/llvm/DebugInfo/DWARF/DWARFDie.h
391

I would rather not tackle reconstituting the NULL Die in this patch. I would rather do that if we remove the NULL dies. I think it is fair to say that when iterating across the children that we skip the NULL DIEs. I can fix the issue where we have only a NULL DIE as the only child.

397

If I return a "const DWARFDie &" from operator* I get:

/Volumes/Data/Users/gclayton/Documents/src/llvm/tot/llvm/include/llvm/ADT/iterator.h:138:12: error: cannot initialize return object of type 'llvm::DWARFDie *' with an rvalue of type 'const llvm::DWARFDie *'

For the following code in iterator.h:

PointerT operator->() const {
  return &static_cast<const DerivedT *>(this)->operator*();
}

If I make operator* return a "DWARFDie &" then it works and I can remove operator->:

DWARFDie &operator*() const { return const_cast<DWARFDie &>(Die); }

Without the cast it complains about a const function converting to a non const DWARFDie &. Which solution do you prefer?

410–412

It can't it needs to know what a DWARFDie::iterator is. I tried but got a compile error.

clayborg updated this revision to Diff 83281.Jan 5 2017, 11:30 AM

Changed over to using "const DWARFDie" as the template parameter to iterator_facade_base so we don't have to override the operator->.

All issues should be resolved.

dblaikie added inline comments.Jan 5 2017, 11:32 AM
include/llvm/DebugInfo/DWARF/DWARFDie.h
392

Is this tested?

clayborg updated this revision to Diff 83304.Jan 5 2017, 1:59 PM

Added a test to test Null DIEs being constructed into a DWARFDie::iterator to ensure it skips the NULL die and is the same as the end iterator.

Next I will get back to removing the NULL dies as a patch to clean things up.

include/llvm/DebugInfo/DWARF/DWARFDie.h
392

I can add a test for it.

dblaikie added inline comments.Jan 5 2017, 2:07 PM
unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1210–1228

I would expect this test case (or the one above (TestChildIterators)) to have a DIE with a zero length children list, rather than the explicit test at the end here which looks more like it's testing the internals of DWARFDie::iterator (that iterator(DWARFDie) ctor should probably not be public and instead only accessible from DWARFDie).

Is there a way to create a test that parses a DIE with a zero-length (but present) child list instead? Perhaps if it's not possible with the DWARF APIs we have, Chris Bieneman's yaml2obj might be up to the task.

clayborg added inline comments.Jan 5 2017, 2:35 PM
unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1228

It isn't possible with the DWARF APIs currently without adding code that no one would want.

I was able to use DwarfGenerator to generate a DWARF file that was close that I saved to disk. I then used obj2yaml to make a yaml file, edited it and got what we need:

--- !mach-o
FileHeader:      
  magic:           0xFEEDFACF
  cputype:         0x01000007
  cpusubtype:      0x00000003
  filetype:        0x00000001
  ncmds:           2
  sizeofcmds:      248
  flags:           0x00000000
  reserved:        0x00000000
LoadCommands:    
  - cmd:             LC_SEGMENT_64
    cmdsize:         232
    segname:         ''
    vmaddr:          0
    vmsize:          27
    fileoff:         280
    filesize:        27
    maxprot:         7
    initprot:        7
    nsects:          2
    flags:           0
    Sections:        
      - sectname:        __debug_abbrev
        segname:         __DWARF
        addr:            0x0000000000000000
        size:            5
        offset:          0x00000118
        align:           0
        reloff:          0x00000000
        nreloc:          0
        flags:           0x02000000
        reserved1:       0x00000000
        reserved2:       0x00000000
        reserved3:       0x00000000
      - sectname:        __debug_info
        segname:         __DWARF
        addr:            0x000000000000000D
        size:            14
        offset:          0x00000125
        align:           0
        reloff:          0x00000000
        nreloc:          0
        flags:           0x02000000
        reserved1:       0x00000000
        reserved2:       0x00000000
        reserved3:       0x00000000
  - cmd:             LC_VERSION_MIN_MACOSX
    cmdsize:         16
    version:         1048576
    sdk:             0
DWARF:           
  debug_abbrev:    
    - Code:            0x00000001
      Tag:             DW_TAG_compile_unit
      Children:        DW_CHILDREN_yes
      Attributes:      
  debug_info:      
    - Length:          10
      Version:         4
      AbbrOffset:      0
      AddrSize:        8
      Entries:         
        - AbbrCode:        0x00000001
          Values:          
        - AbbrCode:        0x00000000
          Values:          
...

Are you OK if I have this text in a "const char *" variable and use the yaml2obj APIs to create an in memory file and then parse that DWARF and then test this? I can't really use FileCheck to test this internal iteration API. Let me know if you are ok with this approach?

Looks like the guts to yaml2obj is only available as a command line tool and won't be available to use in the DWARF unit test binary.

Are there any unit tests that spawn child processes to do work? That is what I would need to do to make this work.

clayborg updated this revision to Diff 83315.Jan 5 2017, 3:27 PM

Fixed the DIE class to allow it generate an abbreviation that says the DIE has children even when it doesn't. This allows us to properly test the child iteration for DIEs that say they have kids, but don't.

Added the ability to generate a DIE with children even when it doesn't turned out to be pretty easy.

dblaikie accepted this revision.Jan 5 2017, 3:46 PM
dblaikie edited edge metadata.

One minor comment.

Also would still love to hear from Chris about testing more interesting cases of non-trivial DWARF and API interactions. I mean perhaps we'll end up with all parsing failures testable with llvm-dwarfdump and something like obj2yaml, then all API interactions we'll test with the DWARF generation APIs and make sure they can express all valid DWARF. (though I'm wondering at that point if it wouldn't be better for us to just create the DWARF objects directly - in fact, in this case we could do that, perhaps - rather than parsing DWARF, we could just create the DWARF parser API objects directly and check that the accessors all work, etc)

unittests/DebugInfo/DWARF/DwarfGenerator.cpp
112 ↗(On Diff #83315)

Is this conditional needed - or do we generally assume it's an invariant that the Die is non-null for most of the operations on dwarfgen::DIE?

This revision is now accepted and ready to land.Jan 5 2017, 3:46 PM
clayborg added inline comments.Jan 5 2017, 3:57 PM
unittests/DebugInfo/DWARF/DwarfGenerator.cpp
112 ↗(On Diff #83315)

I'll take the if out.

This revision was automatically updated to reflect the committed changes.