This is an archive of the discontinued LLVM Phabricator instance.

2/2: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE
ClosedPublic

Authored by jankratochvil on Apr 12 2020, 3:41 AM.

Details

Summary

@labath was saying in D73206:

And /I think/ we could make it interface returns DWARFDies directly

Diff Detail

Event Timeline

jankratochvil created this revision.Apr 12 2020, 3:41 AM
jankratochvil marked 2 inline comments as done.Apr 12 2020, 3:45 AM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
82

This calling with curly brackets is a bit tricky but I found it the least worst option.

93

Invalid DIERef is now reported for all Indexes, not just AppleDWARFIndex.

Looks pretty good, I just think the std::function solution is too smart for its own good.

lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
82

I think it would be much simpler to make this a callable object instead (though I'm not sure we need to worry about optimizing to this level of detail):

class ResultProcessor (?) {
  ResultProcessor(callback, name);
  bool operator()(DIERef ref);
};
lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
186

dangling DIERefCallbackArgs here.

jankratochvil marked 4 inline comments as done.Apr 21 2020, 1:12 AM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
82

Wow, that solved everything, thanks. No more mallocs, no more stale references, the most simple calling syntax. No compromises.
I did not know llvm::function_ref<> does not need any std::function, it is fine with a callable object.

lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
186

oops, I even did not handle the usage in a patch of mine. But the stale references are no longer possible with the callable object now.

jankratochvil marked 2 inline comments as done.
labath accepted this revision.Apr 22 2020, 5:00 AM

Looks good, just please write more explanatory commit message than what's present in this patch description.

lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
75–80

reverse the if condition

if (DWARFDIE die = m_dwarf.GETDIE(ref))
  return m_callback(die);
...
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
93

That's fine. The error message that produces is really geared towards accelerator tables, but the manual index should not produce such an error anyway (I would have put an assert there it was convenient (but it isn't)). And I think I'm going to change the debug_names index to not use DIERefs once this lands.

This revision is now accepted and ready to land.Apr 22 2020, 5:00 AM
jankratochvil marked 2 inline comments as done.Apr 22 2020, 7:40 AM

Thanks for the review and I have learned a new use case for llvm::function_ref.

This revision was automatically updated to reflect the committed changes.