[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil requested changes to this revision.
jankratochvil added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/cmake/modules/FindDebuginfod.cmake:59
+endif()
\ No newline at end of file

----------------
"No newline at end of file", this is what saving this diff, git apply --index and git diff says to me.


================
Comment at: lldb/include/lldb/Host/DebugInfoD.h:27
+// cached version of the file. If there  was an error, we return that instead.
+llvm::Expected<std::string> findSource(const UUID &buildID,
+                                       const std::string &path);
----------------
Describe what does mean a returned `std::string("")` - that no error happened but server does not know this UUID/path.


================
Comment at: lldb/source/Core/SourceManager.cpp:408
+      if ((!file_spec.GetDirectory() && file_spec.GetFilename()) ||
+          !FileSystem::Instance().Exists(m_file_spec)) {
         // If this is just a file name, lets see if we can find it in the
----------------
I do not like this extra line as it changes behavior of LLDB unrelated to `debuginfod`. IIUC if the source file with fully specified directory+filename in DWARF does not exist but the same filename exists in a different directory of the sourcetree LLDB will now quietly use the different file. That's a bug.
I think it is there as you needed to initialize `sc.module_sp`.


================
Comment at: lldb/source/Core/SourceManager.cpp:460
+      if (!FileSystem::Instance().Exists(m_file_spec) &&
+          debuginfod::isAvailable() && sc.module_sp) {
+        llvm::Expected<std::string> cache_path = debuginfod::findSource(
----------------
Make the `debuginfod::isAvailable()` check first as it is zero-cost, `FileSystem::Instance().Exists` is expensive filesystem operation.
The problem with that `sc.module_sp` is it is initialized above with some side effects. I think you should be fine without needing any `sc`. The following code does not pass the testcase for me but I guess you may fix it better:
```
      // Try finding the file using elfutils' debuginfod
      if (!FileSystem::Instance().Exists(m_file_spec) &&
          debuginfod::isAvailable())
        target->GetImages().ForEach(
            [&](const ModuleSP &module_sp) -> bool {
          llvm::Expected<std::string> cache_path = debuginfod::findSource(
              module_sp->GetUUID(), file_spec.GetCString());
          if (!cache_path) {
            module_sp->ReportWarning(
                "An error occurred while finding the "
                "source file %s using debuginfod for build ID %s: %s",
                file_spec.GetCString(),
                sc.module_sp->GetUUID().GetAsString("").c_str(),
                llvm::toString(cache_path.takeError()).c_str());
          } else if (!cache_path->empty()) {
            m_file_spec = FileSpec(*cache_path);
            m_mod_time = FileSystem::Instance().GetModificationTime(*cache_path);
            return false;
          }
          return true;
        });
```



================
Comment at: lldb/source/Host/common/DebugInfoD.cpp:50
+                                   "invalid build ID: %s",
+                                   buildID.GetAsString("").c_str());
+
----------------
It should not be an error:
```
echo 'int main(void) { return 0; }' >/tmp/main2.c;gcc -o /tmp/main2 /tmp/main2.c -Wall -g -Wl,--build-id=none;rm /tmp/main2.c;DEBUGINFOD_URLS=http://localhost:8002/ ./bin/lldb /tmp/main2 -o 'l main' -o q
(lldb) target create "/tmp/main2"
Current executable set to '/tmp/main2' (x86_64).
(lldb) l main
warning: (x86_64) /tmp/main2 An error occurred while finding the source file /tmp/main2.c using debuginfod for build ID A9C3D738: invalid build ID: A9C3D738
File: /tmp/main2.c
(lldb) q
```



================
Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:103
+// TEST-3: File: /my/new/path/test.cpp
+// TEST-3:         123
+// TEST-3-NEXT:    {{[0-9]+}}   // Some context lines before
----------------
`s/123/{{[0-9]+}}/?`


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:136
+// the function.
\ No newline at end of file

----------------
"No newline at end of file", this is what saving this diff, git apply --index and git diff says to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750