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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
