SymbolVendorELF: Perform build-id lookup even without a debug link

[labath]

Summary

The debug link and build-id lookups are two independent ways one can
search for a separate symbol file. However, our implementation in
SymbolVendorELF was tying the two together and refusing to look up the
symbol file based on a build id if the file did not contain a debug
link.

This patch makes it possible to search for the symbol file with
just one of the two methods available. To demonstrate, I split the
build-id-case test into two, so that we test the search using both
methods.

[jankratochvil]

I find it as a performance regression that even with an unstripped full debug info file (clang -g) LLDB has now started to look for the separate debug info file based on its build-id:

stat("/home/jkratoch/t/.build-id/f9/bf63a422425da856baacfadaa4a4afe43c991c.debug", 0x7ffe7f496470) = -1 ENOENT (No such file or directory)
stat("/home/jkratoch/t/.build-id/f9/bf63a422425da856baacfadaa4a4afe43c991c.debug", 0x7ffe7f4968d0) = -1 ENOENT (No such file or directory)
stat("/home/jkratoch/t/.build-id/f9/bf63a422425da856baacfadaa4a4afe43c991c.debug", 0x7ffe7f496690) = -1 ENOENT (No such file or directory)
stat("/usr/lib/debug/.build-id/f9/bf63a422425da856baacfadaa4a4afe43c991c.debug", 0x7ffe7f496470) = -1 ENOENT (No such file or directory)
stat("/usr/lib/debug/.build-id/f9/bf63a422425da856baacfadaa4a4afe43c991c.debug", 0x7ffe7f4968d0) = -1 ENOENT (No such file or directory)
stat("/usr/lib/debug/.build-id/f9/bf63a422425da856baacfadaa4a4afe43c991c.debug", 0x7ffe7f496690) = -1 ENOENT (No such file or directory)

Any needless file lookups are reported as a performance problem on NFS-mounted disks.
Existence of .gnu_debuglink is considered as a flag marking files which did have debug info which is now split out.
Maybe you could explain what is the real goal.

[labath]

The "real" goal is being able to search for external debug files using the build-id without them having the gnu_debuglink thingy. :)

That would seem to at odds with your " .gnu_debuglink is considered as a flag" assertion, but I am not sure how you came to believe that. The gdb documentation for separate debug files https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html stops short of saying that the build-id can be used without the debug link, but it does speak of them as two "methods" for finding external debug info, which at least to me, is a pretty clear indication that they can be used independently. And indeed, if you try to create such a situation, gdb will happily load the debug info from the separate file based on the build id alone:

/tmp/B $ readelf -S a.out | grep gnu # no .gnu_debuglink
  [ 2] .note.gnu.build-i NOTE             00000000000002c4  000002c4
  [ 4] .gnu.hash         GNU_HASH         0000000000000308  00000308
  [ 7] .gnu.version      VERSYM           0000000000000498  00000498
  [ 8] .gnu.version_r    VERNEED          00000000000004a8  000004a8
/tmp/B $ gdb
GNU gdb (Gentoo 8.3 vanilla) 8.3
...
(gdb) set debug-file-directory /tmp/B/D
(gdb) file a.out
Reading symbols from a.out...
Reading symbols from /tmp/B/D/.build-id/7e/d1cb4d4300b8ff67f2f2ef5b273666e339a8ba.debug... # <== symbols found here

That said, I agree the extra filesystem accesses are bad, and they are a completely unintended side-effect of this patch. If that is the only issue, then I think this can be easily fixed by first checking whether the main object file contains any debug info, and skipping the rest of the lookup in that case.

[jankratochvil]

In D65561#1614293, @labath wrote:

That would seem to at odds with your " .gnu_debuglink is considered as a flag" assertion, but I am not sure how you came to believe that.

I see I remembered it wrongly, already the [[ https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=77069918ac29da24f1ae0e388cef2ea5045397fd | initial build-id implementation for GDB ignored .gnu_debuglink ]].

That said, I agree the extra filesystem accesses are bad, and they are a completely unintended side-effect of this patch. If that is the only issue, then I think this can be easily fixed by first checking whether the main object file contains any debug info, and skipping the rest of the lookup in that case.

Yes, this is what GDB does:

  /* If the file has its own symbol tables it has no separate debug
     info.  `.dynsym'/`.symtab' go to MSYMBOLS, `.debug_info' goes to
     SYMTABS/PSYMTABS.  `.gnu_debuglink' may no longer be present with
     `.note.gnu.build-id'.
...
  else if (!objfile_has_partial_symbols (objfile)   
      std::string debugfile = find_separate_debug_file_by_buildid (objfile);

[labath]

• don't go off searching for separate files if the main one already contains debug info.

lldb/trunk/lit/Modules/ELF/build-id-case.yaml

 # RUN: mkdir -p %t/.build-id/1b
 # RUN: yaml2obj %s > %t/.build-id/1b/8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9.debug
 # RUN: cd %t
-# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=.build-id/1b/8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9.debug %t/.build-id/1b/8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9.debug %t/stripped.out
+# RUN: llvm-objcopy --strip-all %t/.build-id/1b/8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9.debug %t/stripped.out
 # RUN: lldb-test object-file %t/stripped.out | FileCheck %s
 
 # CHECK: Name: .debug_frame
 # CHECK-NEXT: Type: dwarf-frame
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64

lldb/trunk/lit/Modules/ELF/gnu-debuglink.yaml

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-all --add-gnu-debuglink=%t %t %t.stripped
+# RUN: lldb-test object-file %t.stripped | FileCheck %s
+
+# CHECK: Name: .debug_frame
+# CHECK-NEXT: Type: dwarf-frame
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x00000000004003D0
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x00000000004003D0
+    AddressAlign:    0x0000000000000010
+    Content:         DEADBEEFBAADF00D
+  - Name:            .debug_frame
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000008
+    Content:         DEADBEEFBAADF00D
+Symbols:
+  - Name:            main
+    Type:            STT_FUNC
+    Section:         .text
+    Value:           0x00000000004003D0
+    Size:            0x0000000000000008
+...

lldb/trunk/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp

ObjectFileELF *obj_file =
       llvm::dyn_cast_or_null<ObjectFileELF>(module_sp->GetObjectFile());
   if (!obj_file)
     return nullptr;
 
   lldb_private::UUID uuid = obj_file->GetUUID();
   if (!uuid)
     return nullptr;
 
+  // If the main object file already contains debug info, then we are done.
+  if (obj_file->GetSectionList()->FindSectionByType(
+          lldb::eSectionTypeDWARFDebugInfo, true))
+    return nullptr;
+
   // If the module specified a filespec, use that.
   FileSpec fspec = module_sp->GetSymbolFileFileSpec();
   // Otherwise, try gnu_debuglink, if one exists.
   if (!fspec)
     fspec = obj_file->GetDebugLink().getValueOr(FileSpec());
 
-  // If we have no debug symbol files, then nothing to do.
-  if (!fspec)
-    return nullptr;
-
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(func_cat, "SymbolVendorELF::CreateInstance (module = %s)",
                      module_sp->GetFileSpec().GetPath().c_str());
 
   ModuleSpec module_spec;
 
   module_spec.GetFileSpec() = obj_file->GetFileSpec();
   FileSystem::Instance().Resolve(module_spec.GetFileSpec());

lldb/trunk/source/Symbol/LocateSymbolFile.cpp

 FileSpec
 Symbols::LocateExecutableSymbolFile(const ModuleSpec &module_spec,
                                     const FileSpecList &default_search_paths) {
   FileSpec symbol_file_spec = module_spec.GetSymbolFileSpec();
   if (symbol_file_spec.IsAbsolute() &&
       FileSystem::Instance().Exists(symbol_file_spec))
     return symbol_file_spec;
 
-  const char *symbol_filename = symbol_file_spec.GetFilename().AsCString();
-  if (symbol_filename && symbol_filename[0]) {
-    FileSpecList debug_file_search_paths = default_search_paths;
 
-    // Add module directory.
-    FileSpec module_file_spec = module_spec.GetFileSpec();
-    // We keep the unresolved pathname if it fails.
-    FileSystem::Instance().ResolveSymbolicLink(module_file_spec,
-                                               module_file_spec);
 
-    ConstString file_dir = module_file_spec.GetDirectory();
-    {
-      FileSpec file_spec(file_dir.AsCString("."));
-      FileSystem::Instance().Resolve(file_spec);
-      debug_file_search_paths.AppendIfUnique(file_spec);
-    }
 
-    if (ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup()) {
 
-      // Add current working directory.
-      {
-        FileSpec file_spec(".");
-        FileSystem::Instance().Resolve(file_spec);
-        debug_file_search_paths.AppendIfUnique(file_spec);
-      }
 
 #ifndef _WIN32
 #if defined(__NetBSD__)
-      // Add /usr/libdata/debug directory.
-      {
-        FileSpec file_spec("/usr/libdata/debug");
-        FileSystem::Instance().Resolve(file_spec);
-        debug_file_search_paths.AppendIfUnique(file_spec);
-      }
 #else
-      // Add /usr/lib/debug directory.
-      {
-        FileSpec file_spec("/usr/lib/debug");
-        FileSystem::Instance().Resolve(file_spec);
-        debug_file_search_paths.AppendIfUnique(file_spec);
-      }
 #endif
 #endif // _WIN32
-    }
 
-    std::string uuid_str;
-    const UUID &module_uuid = module_spec.GetUUID();
-    if (module_uuid.IsValid()) {
-      // Some debug files are stored in the .build-id directory like this:
-      //   /usr/lib/debug/.build-id/ff/e7fe727889ad82bb153de2ad065b2189693315.debug
-      uuid_str = module_uuid.GetAsString("");
-      std::transform(uuid_str.begin(), uuid_str.end(), uuid_str.begin(),
-                     ::tolower);
-      uuid_str.insert(2, 1, '/');
-      uuid_str = uuid_str + ".debug";
-    }
 
-    size_t num_directories = debug_file_search_paths.GetSize();
-    for (size_t idx = 0; idx < num_directories; ++idx) {
-      FileSpec dirspec = debug_file_search_paths.GetFileSpecAtIndex(idx);
-      FileSystem::Instance().Resolve(dirspec);
-      if (!FileSystem::Instance().IsDirectory(dirspec))
-        continue;
 
-      std::vector<std::string> files;
-      std::string dirname = dirspec.GetPath();
 
-      files.push_back(dirname + "/" + symbol_filename);
-      files.push_back(dirname + "/.debug/" + symbol_filename);
+    if (!uuid_str.empty())
       files.push_back(dirname + "/.build-id/" + uuid_str);
+    if (symbol_file_spec.GetFilename()) {
+      files.push_back(dirname + "/" +
+                      symbol_file_spec.GetFilename().GetCString());
+      files.push_back(dirname + "/.debug/" +
+                      symbol_file_spec.GetFilename().GetCString());
 
       // Some debug files may stored in the module directory like this:
       //   /usr/lib/debug/usr/lib/library.so.debug
       if (!file_dir.IsEmpty())
-        files.push_back(dirname + file_dir.AsCString() + "/" + symbol_filename);
+        files.push_back(dirname + file_dir.AsCString() + "/" +
+                        symbol_file_spec.GetFilename().GetCString());
+    }
 
-      const uint32_t num_files = files.size();
-      for (size_t idx_file = 0; idx_file < num_files; ++idx_file) {
-        const std::string &filename = files[idx_file];
-        FileSpec file_spec(filename);
-        FileSystem::Instance().Resolve(file_spec);
 
-        if (llvm::sys::fs::equivalent(file_spec.GetPath(),
-                                      module_file_spec.GetPath()))
-          continue;
 
-        if (FileSystem::Instance().Exists(file_spec)) {
-          lldb_private::ModuleSpecList specs;
-          const size_t num_specs =
-              ObjectFile::GetModuleSpecifications(file_spec, 0, 0, specs);
-          assert(num_specs <= 1 &&
-                 "Symbol Vendor supports only a single architecture");
-          if (num_specs == 1) {
-            ModuleSpec mspec;
-            if (specs.GetModuleSpecAtIndex(0, mspec)) {
-              // Skip the uuids check if module_uuid is invalid. For example,
-              // this happens for *.dwp files since at the moment llvm-dwp
-              // doesn't output build ids, nor does binutils dwp.
-              if (!module_uuid.IsValid() || module_uuid == mspec.GetUUID())
-                return file_spec;
-            }
-          }
-        }
-      }
-    }
-  }
 
   return LocateExecutableSymbolFileDsym(module_spec);
 }
 
 #if !defined(__APPLE__)
 
 FileSpec Symbols::FindSymbolFileInBundle(const FileSpec &symfile_bundle,
                                          const lldb_private::UUID *uuid,