[Lldb-commits] [PATCH] D77327: 1/2: [nfc] [lldb] Introduce DWARF callbacks

Tue Apr 14 02:37:11 PDT 2020

labath added a comment.

In D77327#1971794 <https://reviews.llvm.org/D77327#1971794>, @jankratochvil wrote:

> In D77327#1971435 <https://reviews.llvm.org/D77327#1971435>, @labath wrote:
>
> > 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77327/new/

https://reviews.llvm.org/D77327