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 lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits