Page MenuHomePhabricator

[SymbolFile] Implement GetCompleteObjCClass for .debug_names
ClosedPublic

Authored by JDevlieghere on Jun 26 2018, 8:49 AM.

Details

Summary

When running the test suite with .debug_names a bunch of tests were failing. Part of that is solved by implementing the missing GetCompleteObjCClass in DebugNamesDWARFIndex.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jun 26 2018, 8:49 AM
aprantl added inline comments.Jun 26 2018, 9:02 AM
source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
157 ↗(On Diff #152898)

Wait.. we accept both structs and classes in LLDB? Should we also switch over to using DW_TAG_class in clang since that sounds more appropriate anyway?

clayborg requested changes to this revision.Jun 26 2018, 9:28 AM

The function is looking for the complete objective C type. The code needs to be modified to return the complete type only if one is found, else just one of the other incomplete versions is needed, not many.

source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
166 ↗(On Diff #152898)

Why are we pushing a potentially incomplete ObjC type here? We should keep a list of incomplete types and if we don't find a complete one, then return that as a fallback.

173 ↗(On Diff #152898)

Why are pushing a DIERef that doesn't produce a DIE? Remove the push_back.

178 ↗(On Diff #152898)

If we find this, return right away with only one result: the complete version.

180 ↗(On Diff #152898)

We need to add code here to return an incomplete version of the ObjC type from line 166 if no complete versions are found.

This revision now requires changes to proceed.Jun 26 2018, 9:28 AM

The function is looking for the complete objective C type. The code needs to be modified to return the complete type only if one is found, else just one of the other incomplete versions is needed, not many.

Thanks Greg, looks like I misunderstood the code. I'll update the patch.

JDevlieghere marked an inline comment as done.
  • Feedback Greg
source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
157 ↗(On Diff #152898)

I'm probably not the right person to answer this. I copied this from HashedNameToDIE:

611       // We don't only want the one true definition, so try and see what we can                                                                                                                                                                                                                                                                  612       // find, and only return class or struct DIEs.
...
616       DWARFMappedHash::ExtractClassOrStructDIEArray(

How are you planning on testing this? Most of the DWARFIndex functionality is tested in terms of lldb-test, but I think this function is not reachable from there. Maybe you could write a specific dotest-test which targets this?

  • Add test case for debug_names variant.
labath accepted this revision.Jun 27 2018, 4:28 AM

looks good to me

clayborg accepted this revision.Jun 27 2018, 7:20 AM

Looks good.

This revision is now accepted and ready to land.Jun 27 2018, 7:20 AM
This revision was automatically updated to reflect the committed changes.