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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.


================
Comment at: lldb/source/Core/SourceManager.cpp:415
             target->GetImages().ResolveSymbolContextForFilePath(
                 file_spec.GetFilename().AsCString(), 0, check_inlines,
                 SymbolContextItem(eSymbolContextModule |
----------------
This code could be more efficient than my previously proposed `GetImages.ForEach()` as it should be able to find the only one `Module` having that source file.
But there should be passed the full pathname incl. directories to prevent wrongly chosen accidentally filename-matching source files:
```
FileSystem::Instance().Exists(m_file_spec) ? file_spec.GetFilename().AsCString() : file_spec.GetCString(false/*denormalize*/)
```
And the `Exists()` check should be cached in this whole function as it is expensive.



================
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(
----------------
jankratochvil wrote:
> 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;
>         });
> ```
> 
Please ignore this comment + code fragment, I think it should not be needed.
(Just the `isAvailable()` check should be moved.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750