This is an archive of the discontinued LLVM Phabricator instance.

1/2: [nfc] [lldb] Introduce DWARF callbacks
ClosedPublic

Authored by jankratochvil on Apr 2 2020, 11:55 AM.

Diff Detail

Event Timeline

jankratochvil created this revision.Apr 2 2020, 11:55 AM
jankratochvil marked 3 inline comments as done.Apr 2 2020, 12:02 PM
jankratochvil added inline comments.
lldb/include/lldb/Core/UniqueCStringMap.h
138

Semantics of the callback is apparent - return true if the caller no longer needs any more data. Not sure where to document that, maybe it is simple enough in the code.

lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
69–70

DIEInfoArray is not callback-optimized in this patch. That could be done as an add-on patch.

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
84

The meaning of return_implementation_only_if_available has changed here. The only caller was using it with true so now the only existing caller calls it twice - first with true and then with false as we can no longer do the original die_offsets.clear();.

jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2385

This is not NFC but it is required by the refactorization. I have posted this bugfix separately as D77336.

jankratochvil retitled this revision from [lldb] [almost-nfc] 2/2: Introduce DWARF callbacks to [nfc] [lldb] 2/2: Introduce DWARF callbacks.Apr 2 2020, 2:12 PM
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2385

It has been now fixed by checked-in D77336.

labath added a comment.Apr 6 2020, 2:30 AM

Thanks for doing this. This looks more-or-less like what I was expecting. The need for the "bool" return value from the index functions is unfortunate (I did not anticipate that), but it does seem unavoidable.

Since none of these functions (I guess) are storing the callback function, could you replace std::function with llvm::function_ref ?

lldb/include/lldb/Core/UniqueCStringMap.h
134–167

I'm not sure these functions are really needed. They have just one caller, and they'd be trivial if this class provided appropriate abstractions. Maybe just add begin/end/equal_range methods, so that one can write:

for (value: map / map.equal_range(str))
  stuff

?

(ideally, I'd like to have iterators instead of callbacks for the index classes too, but iterators and class hierarchies don't mix very well)

lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
83–84

Maybe rename this to ProcessEntry or something?

jankratochvil marked 2 inline comments as done.Apr 6 2020, 7:44 AM
jankratochvil added inline comments.
lldb/include/lldb/Core/UniqueCStringMap.h
134–167

Do you mean the standard iteration:

for (std::pair<ConstString, DIERef> pair : map)
  stuff;

or really

for (DIERef ref : map)
  stuff;

?

jankratochvil planned changes to this revision.Apr 6 2020, 11:04 AM
jankratochvil marked 2 inline comments as done.Apr 6 2020, 1:18 PM
jankratochvil added inline comments.
lldb/include/lldb/Core/UniqueCStringMap.h
134–167

Implemented. IIUC for regexes it should be matched in a caller - or to make also an iterator for regexes?

for (const auto &entry : m_map.equal_range(name))
  if (callback(entry.value))
for (const auto &entry : m_map)
  if (regex.Execute(entry.cstring.GetCString())) {
    if (callback(entry.value))
aprantl added inline comments.Apr 6 2020, 2:06 PM
lldb/include/lldb/Core/UniqueCStringMap.h
105

///

shafik added inline comments.Apr 6 2020, 4:08 PM
lldb/include/lldb/Core/UniqueCStringMap.h
106

CStringIterator?

137

It feels like this should be a pointer, m_entry_ptr is a pointer already and most uses in the class use the pointer value already.

labath added a comment.Apr 7 2020, 8:34 AM

I can't say I have tried this, but ummm.. why is there a need for a separate iterator class for this equal_range stuff? Couldn't this be implemented via an iterator_pair of the iterators of the underlying container (found via lower/upper_bound) ?

lldb/include/lldb/Core/UniqueCStringMap.h
134–167

Yes, that is correct.

I can't say I have tried this, but ummm.. why is there a need for a separate iterator class for this equal_range stuff? Couldn't this be implemented via an iterator_pair of the iterators of the underlying container (found via lower/upper_bound) ?

OK, true, I will rework it.

jankratochvil planned changes to this revision.Apr 7 2020, 8:52 AM
jankratochvil marked 6 inline comments as done.Apr 7 2020, 12:06 PM
jankratochvil added inline comments.
lldb/include/lldb/Core/UniqueCStringMap.h
105

I am sorry but all comments in this file are // . I can make a conversion of the file to Doxygen in a separate file if you wish but I am not sure how much is Doxygen widespread in LLDB etc.

106

This custom iterator has been now dropped upon advice by @labath the standard iterator is enough.

137

This custom iterator has been now dropped upon advice by @labath the standard iterator is enough.
BTW I have seen patches all pointers which cannot be nullptr to be converted to references which is why I used the reference here. But it is gone now anyway.

jankratochvil marked 2 inline comments as done.
labath added inline comments.Apr 7 2020, 11:27 PM
lldb/include/lldb/Core/UniqueCStringMap.h
190–195

Looks better, but I have a feeling it could be simplified even further. Wouldn't a plain return llvm::make_range(std::equal_range(m_map.begin(), m_map.end(), Compare()); work just as well? (Sorry for taking you down the wrong path with the lower/upper_bound comment -- equal_range is basically a combination of lower_bound and upper_bound calls.)

jankratochvil marked 2 inline comments as done.
jankratochvil added inline comments.
lldb/include/lldb/Core/UniqueCStringMap.h
190–195

Yes, sorry, I was never using these functions before.

labath added a comment.Apr 9 2020, 1:47 AM

I just had one more idea. The bool return value on all of these methods, is that here just to implement the fallback mechanism in DebugNamesDWARFIndex? Could it be avoided if debug_names index calls the "fallback" *after* it has done its own processing?

jankratochvil marked an inline comment as done.

The bool return value on all of these methods, is that here just to implement the fallback mechanism in DebugNamesDWARFIndex?

Most of them yes. There are still some functions needing to return bool to benefit from the early returns from callbacks. I hope I found the minimal set of such bool functions (none of those are in DWARFIndex).

Could it be avoided if debug_names index calls the "fallback" *after* it has done its own processing?

Yes. Done so now. I still do not understand what is DebugNamesDWARFIndex good for when it always calls ManualDWARFIndex anyway.

jankratochvil retitled this revision from [nfc] [lldb] 2/2: Introduce DWARF callbacks to 1/2: [nfc] [lldb] Introduce DWARF callbacks.Apr 12 2020, 3:37 AM

The bool return value on all of these methods, is that here just to implement the fallback mechanism in DebugNamesDWARFIndex?

Most of them yes. There are still some functions needing to return bool to benefit from the early returns from callbacks. I hope I found the minimal set of such bool functions (none of those are in DWARFIndex).

Cool. I am very sorry about the back-and-forth but I just noticed another issue. The normal semantics of callbacks like these is to return true when one wants to continue iterating (e.g. [[ https://github.com/llvm/llvm-project/blob/cdc514e4c67f268b07863bbac3d8d7e0d088186c/lldb/include/lldb/Core/MappedHash.h#L293 | here ]). This patch has the meanings reversed. I think it would be good to maintain the current convention.

Otherwise this looks good, and I really hope this will be the last iteration.

Could it be avoided if debug_names index calls the "fallback" *after* it has done its own processing?

Yes. Done so now. I still do not understand what is DebugNamesDWARFIndex good for when it always calls ManualDWARFIndex anyway.

One cannot guarantee that the debug_names section will cover all compilation units in a given file. So we still need to call the manual index to index the rest. However we pass it a list of units it should avoid (see DebugNamesDWARFIndex constructor), so we don't index everything twice.

Theoretically the calls to the fallback index could be guarded by an if (debug_names_really_covers_everything) check, but that wouldn't really help anything -- in that case the manual index would index an empty set of units and so all calls to it would be a quick no-op anyway.

The normal semantics of callbacks like these is to return true when one wants to continue iterating

OK, fixed, thanks for catching it.

One cannot guarantee that the debug_names section will cover all compilation units in a given file. So we still need to call the manual index to index the rest. However we pass it a list of units it should avoid (see DebugNamesDWARFIndex constructor), so we don't index everything twice.

OK, that llvm::DenseSet<dw_offset_t> units_to_avoid. Thanks for the explanation, it makes sense now.

labath accepted this revision.Apr 15 2020, 12:23 AM

Cool.

This revision is now accepted and ready to land.Apr 15 2020, 12:23 AM
This revision was automatically updated to reflect the committed changes.
jankratochvil reopened this revision.Apr 15 2020, 6:36 AM

I had to revert it: rG9289f34390da
It has caused a regression: lang/cpp/accelerator-table/TestCPPAccelerator.py

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/15323/consoleFull
This revision is now accepted and ready to land.Apr 15 2020, 6:36 AM
jankratochvil planned changes to this revision.Apr 15 2020, 6:36 AM
This revision is now accepted and ready to land.Apr 15 2020, 1:39 PM
jankratochvil marked an inline comment as done.Apr 15 2020, 1:41 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
141

I have found a bug/regression here, during the last check-in there was:

if (!m_apple_types_up->FindByName(context[1].name,

Just I cannot test it now myself without OSX.

mib added a subscriber: mib.Apr 15 2020, 2:01 PM
mib added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
141

Just tried that change on macOS, and this indeed fixes the regression. Just waiting for the test suite run to finish to make sure it didn't break something else.

mib accepted this revision.Apr 15 2020, 2:14 PM

It looks like there's no more regression. LGTM, thanks!

This revision was automatically updated to reflect the committed changes.