[DWARF5] Only fallback to manual index if no entry was found

[kimanh]

Summary

If we succeed at gathering global variables for a compile
unit, there is no need to fallback to generating a manual index.

[kimanh]

Minor: rename variable

[kimanh]

Hello Pavel,

we are currently looking at the usage of the .debug_names section. For DebugNamesDWARFIndex::GetGlobalVariables(DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) we do not see a clear reason why one would need to still call the fallback. In this case, we only look at that particular CU, and if there's already a name index for that CU, going through the name index should be enough? If we are missing something, please let us know! Otherwise, here is a CL that would only conditionally create the manual index.

[jankratochvil]

Is it really just a microoptimization or can you measure any improvement? That ManualDWARFIndex::Index will be called is expected. But there should be m_units_to_avoid covering all the units so it will quickly return without indexing anything:

if (units_to_index.empty())
  return;

That is what happens for me on a trivial example clang -o mainvar mainvar.c -Wall -gdwarf-5 -gpubnames:

   58  	  if (units_to_index.empty())
-> 59  	    return;

Maybe there is rather some other more serious bug to fix (I am aware for example D99850 but that would behave differently).

[kimanh]

Thanks for having a look at the CL!

In D106355#2893959, @jankratochvil wrote:

Is it really just a microoptimization or can you measure any improvement? That ManualDWARFIndex::Index will be called is expected. But there should be m_units_to_avoid covering all the units so it will quickly return without indexing anything:

Yes, we are aware that it filters out the units that are already indexed by the the name index. In our case we have some third-party libraries that were not built by us, and therefore they don't have any name index. Our main focus, is however, not to debug those third party libraries necessarily, but only our main code that we are compiling. Given that the manual index is taking some time to be generated, we could be lazy about generating it only if we need it.

WDYT?

Maybe there is rather some other more serious bug to fix (I am aware for example D99850 but that would behave differently).

No, there's no serious bug that I'm aware of that is linked to this.

[jankratochvil]

In D106355#2896117, @kimanh wrote:

In our case we have some third-party libraries that were not built by us, and therefore they don't have any name index. Our main focus, is however, not to debug those third party libraries necessarily, but only our main code that we are compiling. Given that the manual index is taking some time to be generated, we could be lazy about generating it only if we need it.

I have a feeling during debugging you will hit the ManualDWARFIndex::Index soon during some other access anyway.

One possibility would be to disable loading DWARF for that file. There is settings set symbols.enable-external-lookup false (used by Symbols::LocateExecutableSymbolFile) but that works only for separate debug info in system directories like /usr/lib/debug/.

If you do not want to debug those libraries cannot you just strip debug info from them (llvm-strip -g)?

Wouldn't be best to generate .debug_names for those 3rd party libraries? GDB has gdb-add-index but its .gdb_index has not enough information to be useful for LLDB. Still LLDB could generate .debug_names out of its internal representation in ManualDWARFIndex. That would be useful also for debugging binaries from non-clang compilers.

[kimanh]

If you do not want to debug those libraries cannot you just strip debug info from them (llvm-strip -g)?

Oh, stripping debug info may work. We're on the Chrome DevTools team and we're working on a DWARF-based debugging extension for C/C++. Ideally we'd like to have as few steps as possible that are required for our users to take, in order to successfully debug their application on the web. We'll keep this in mind.

Wouldn't be best to generate .debug_names for those 3rd party libraries? GDB has gdb-add-index but its .gdb_index has not enough information to be useful for LLDB. Still LLDB could generate .debug_names out of its internal representation in ManualDWARFIndex. That would be useful also for debugging binaries from non-clang compilers.

Ah, to make sure that I understand it correctly: using gdb-add-index would help ManualDWARFIndex to generate a .debug_names?

[jankratochvil]

In D106355#2903655, @kimanh wrote:

Ah, to make sure that I understand it correctly: using gdb-add-index would help ManualDWARFIndex to generate a .debug_names?

No. One could write hypothetical lldb-add-index to generate .debug_names using ManualDWARFIndex. There does not exist such tool yet.

[jankratochvil]

Approved after removing the curly brackets.

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

136

It would be nice to reverse these two comparisons for found_entry_for_cu but getCUOffset() is much more expensive. Just saying.

149–151

[kimanh]

Remove braces.

[kimanh]

Thanks once more for reviewing! If I should rename the variable (or anything else), let me know! Otherwise, could you help me to land this change (since I do not have committer rights)?

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

136

Ah, yes, that's true. I could call it found_variable_entry_for_cu though?

[jankratochvil]

You can also consider coding lldb-add-index for better performance as that is too much for my available time.

In D106355#2909462, @kimanh wrote:

Otherwise, could you help me to land this change (since I do not have committer rights)?

I guess you could already ask for a commit access, you have multiple patches checked in.

[kimanh]

In D106355#2913605, @jankratochvil wrote:

You can also consider coding lldb-add-index for better performance as that is too much for my available time.

I don't think that I'll get to it either. But we'll also evaluate the performance for our users and may need to do more, and in that case we might have a look!

I guess you could already ask for a commit access, you have multiple patches checked in.

Thanks for the suggestion, I just requested it now!

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

void DebugNamesDWARFIndex::GetGlobalVariables(
   }
 
   m_fallback.GetGlobalVariables(regex, callback);
 }
 
 void DebugNamesDWARFIndex::GetGlobalVariables(
     const DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
   uint64_t cu_offset = cu.GetOffset();
+  bool found_entry_for_cu = false;
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
     for (DebugNames::NameTableEntry nte: ni) {
       uint64_t entry_offset = nte.getEntryOffset();
       llvm::Expected<DebugNames::Entry> entry_or = ni.getEntry(&entry_offset);
       for (; entry_or; entry_or = ni.getEntry(&entry_offset)) {
         if (entry_or->tag() != DW_TAG_variable)
           continue;
         if (entry_or->getCUOffset() != cu_offset)
           continue;
 
+        found_entry_for_cu = true;
         if (!ProcessEntry(*entry_or, callback,
                           llvm::StringRef(nte.getString())))
           return;
       }
       MaybeLogLookupError(entry_or.takeError(), ni, nte.getString());
     }
   }
-
+  // If no name index for that particular CU was found, fallback to
+  // creating the manual index.
+  if (!found_entry_for_cu)
     m_fallback.GetGlobalVariables(cu, callback);
 }
 
 void DebugNamesDWARFIndex::GetCompleteObjCClass(
     ConstString class_name, bool must_be_implementation,
     llvm::function_ref<bool(DWARFDIE die)> callback) {
   // Keep a list of incomplete types as fallback for when we don't find the
   // complete type.
   DIEArray incomplete_types;