[lldb] Store SupportFiles in SourceManager::File (NFC) #106639
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To support detecting MD5 checksum mismatches, store a SupportFile rather than a plain FileSpec in SourceManager::File.
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesTo support detecting MD5 checksum mismatches, store a SupportFile rather than a plain FileSpec in SourceManager::File. Full diff: https://github.com/llvm/llvm-project/pull/106639.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Core/SourceManager.h b/lldb/include/lldb/Core/SourceManager.h
index 5239ac6f4055f5..3459a1031a10d7 100644
--- a/lldb/include/lldb/Core/SourceManager.h
+++ b/lldb/include/lldb/Core/SourceManager.h
@@ -37,8 +37,8 @@ class SourceManager {
const SourceManager::File &rhs);
public:
- File(const FileSpec &file_spec, lldb::TargetSP target_sp);
- File(const FileSpec &file_spec, lldb::DebuggerSP debugger_sp);
+ File(lldb::SupportFileSP support_file_sp, lldb::TargetSP target_sp);
+ File(lldb::SupportFileSP support_file_sp, lldb::DebuggerSP debugger_sp);
bool ModificationTimeIsStale() const;
bool PathRemappingIsStale() const;
@@ -56,7 +56,10 @@ class SourceManager {
bool LineIsValid(uint32_t line);
- const FileSpec &GetFileSpec() { return m_file_spec; }
+ lldb::SupportFileSP GetSupportFile() const {
+ assert(m_support_file_sp && "SupportFileSP must always be valid");
+ return m_support_file_sp;
+ }
uint32_t GetSourceMapModificationID() const { return m_source_map_mod_id; }
@@ -70,15 +73,17 @@ class SourceManager {
protected:
/// Set file and update modification time.
- void SetFileSpec(FileSpec file_spec);
+ void SetSupportFile(lldb::SupportFileSP support_file_sp);
bool CalculateLineOffsets(uint32_t line = UINT32_MAX);
- FileSpec m_file_spec_orig; // The original file spec that was used (can be
- // different from m_file_spec)
- FileSpec m_file_spec; // The actually file spec being used (if the target
- // has source mappings, this might be different from
- // m_file_spec_orig)
+ /// The original support file that was used.
+ lldb::SupportFileSP m_original_support_file_sp;
+
+ /// The actually support file being used. If the target
+ /// has source mappings, this might be different from
+ /// the original support file.
+ lldb::SupportFileSP m_support_file_sp;
// Keep the modification time that this file data is valid for
llvm::sys::TimePoint<> m_mod_time;
@@ -93,7 +98,8 @@ class SourceManager {
lldb::TargetWP m_target_wp;
private:
- void CommonInitializer(const FileSpec &file_spec, lldb::TargetSP target_sp);
+ void CommonInitializer(lldb::SupportFileSP support_file_sp,
+ lldb::TargetSP target_sp);
};
typedef std::shared_ptr<File> FileSP;
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index 5ddd46ac5fdc07..1a0629c6765d41 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -1076,8 +1076,8 @@ class CommandObjectSourceList : public CommandObjectParsed {
target.GetSourceManager().GetLastFile());
if (last_file_sp) {
const bool show_inlines = true;
- m_breakpoint_locations.Reset(last_file_sp->GetFileSpec(), 0,
- show_inlines);
+ m_breakpoint_locations.Reset(
+ last_file_sp->GetSupportFile()->GetSpecOnly(), 0, show_inlines);
SearchFilterForUnconstrainedSearches target_search_filter(
target.shared_from_this());
target_search_filter.Search(m_breakpoint_locations);
diff --git a/lldb/source/Core/IOHandlerCursesGUI.cpp b/lldb/source/Core/IOHandlerCursesGUI.cpp
index d922d32f910583..8f44e3d0cd016b 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -6894,8 +6894,8 @@ class SourceFileWindowDelegate : public WindowDelegate {
if (context_changed)
m_selected_line = m_pc_line;
- if (m_file_sp &&
- m_file_sp->GetFileSpec() == m_sc.line_entry.GetFile()) {
+ if (m_file_sp && m_file_sp->GetSupportFile()->GetSpecOnly() ==
+ m_sc.line_entry.GetFile()) {
// Same file, nothing to do, we should either have the lines or
// not (source file missing)
if (m_selected_line >= static_cast<size_t>(m_first_visible_line)) {
@@ -7001,7 +7001,8 @@ class SourceFileWindowDelegate : public WindowDelegate {
LineEntry bp_loc_line_entry;
if (bp_loc_sp->GetAddress().CalculateSymbolContextLineEntry(
bp_loc_line_entry)) {
- if (m_file_sp->GetFileSpec() == bp_loc_line_entry.GetFile()) {
+ if (m_file_sp->GetSupportFile()->GetSpecOnly() ==
+ bp_loc_line_entry.GetFile()) {
bp_lines.insert(bp_loc_line_entry.line);
}
}
@@ -7332,7 +7333,7 @@ class SourceFileWindowDelegate : public WindowDelegate {
if (exe_ctx.HasProcessScope() && exe_ctx.GetProcessRef().IsAlive()) {
BreakpointSP bp_sp = exe_ctx.GetTargetRef().CreateBreakpoint(
nullptr, // Don't limit the breakpoint to certain modules
- m_file_sp->GetFileSpec(), // Source file
+ m_file_sp->GetSupportFile()->GetSpecOnly(), // Source file
m_selected_line +
1, // Source line number (m_selected_line is zero based)
0, // Unspecified column.
@@ -7478,7 +7479,8 @@ class SourceFileWindowDelegate : public WindowDelegate {
LineEntry bp_loc_line_entry;
if (bp_loc_sp->GetAddress().CalculateSymbolContextLineEntry(
bp_loc_line_entry)) {
- if (m_file_sp->GetFileSpec() == bp_loc_line_entry.GetFile() &&
+ if (m_file_sp->GetSupportFile()->GetSpecOnly() ==
+ bp_loc_line_entry.GetFile() &&
m_selected_line + 1 == bp_loc_line_entry.line) {
bool removed =
exe_ctx.GetTargetRef().RemoveBreakpointByID(bp_sp->GetID());
@@ -7492,7 +7494,7 @@ class SourceFileWindowDelegate : public WindowDelegate {
// No breakpoint found on the location, add it.
BreakpointSP bp_sp = exe_ctx.GetTargetRef().CreateBreakpoint(
nullptr, // Don't limit the breakpoint to certain modules
- m_file_sp->GetFileSpec(), // Source file
+ m_file_sp->GetSupportFile()->GetSpecOnly(), // Source file
m_selected_line +
1, // Source line number (m_selected_line is zero based)
0, // No column specified.
diff --git a/lldb/source/Core/SourceManager.cpp b/lldb/source/Core/SourceManager.cpp
index 0d70c554e5342b..75da550314c322 100644
--- a/lldb/source/Core/SourceManager.cpp
+++ b/lldb/source/Core/SourceManager.cpp
@@ -87,8 +87,10 @@ SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
LLDB_LOG(log, "Source file caching disabled: creating new source file: {0}",
file_spec);
if (target_sp)
- return std::make_shared<File>(file_spec, target_sp);
- return std::make_shared<File>(file_spec, debugger_sp);
+ return std::make_shared<File>(std::make_shared<SupportFile>(file_spec),
+ target_sp);
+ return std::make_shared<File>(std::make_shared<SupportFile>(file_spec),
+ debugger_sp);
}
ProcessSP process_sp = target_sp ? target_sp->GetProcessSP() : ProcessSP();
@@ -136,7 +138,8 @@ SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
}
// Check if the file exists on disk.
- if (file_sp && !FileSystem::Instance().Exists(file_sp->GetFileSpec())) {
+ if (file_sp && !FileSystem::Instance().Exists(
+ file_sp->GetSupportFile()->GetSpecOnly())) {
LLDB_LOG(log, "File doesn't exist on disk: {0}", file_spec);
file_sp.reset();
}
@@ -148,9 +151,11 @@ SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
// (Re)create the file.
if (target_sp)
- file_sp = std::make_shared<File>(file_spec, target_sp);
+ file_sp = std::make_shared<File>(std::make_shared<SupportFile>(file_spec),
+ target_sp);
else
- file_sp = std::make_shared<File>(file_spec, debugger_sp);
+ file_sp = std::make_shared<File>(std::make_shared<SupportFile>(file_spec),
+ debugger_sp);
// Add the file to the debugger and process cache. If the file was
// invalidated, this will overwrite it.
@@ -444,25 +449,27 @@ void SourceManager::FindLinesMatchingRegex(FileSpec &file_spec,
match_lines);
}
-SourceManager::File::File(const FileSpec &file_spec,
+SourceManager::File::File(SupportFileSP support_file_sp,
lldb::DebuggerSP debugger_sp)
- : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(),
+ : m_original_support_file_sp(support_file_sp),
+ m_support_file_sp(std::make_shared<SupportFile>()), m_mod_time(),
m_debugger_wp(debugger_sp), m_target_wp(TargetSP()) {
- CommonInitializer(file_spec, {});
+ CommonInitializer(support_file_sp, {});
}
-SourceManager::File::File(const FileSpec &file_spec, TargetSP target_sp)
- : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(),
+SourceManager::File::File(SupportFileSP support_file_sp, TargetSP target_sp)
+ : m_original_support_file_sp(support_file_sp),
+ m_support_file_sp(std::make_shared<SupportFile>()), m_mod_time(),
m_debugger_wp(target_sp ? target_sp->GetDebugger().shared_from_this()
: DebuggerSP()),
m_target_wp(target_sp) {
- CommonInitializer(file_spec, target_sp);
+ CommonInitializer(support_file_sp, target_sp);
}
-void SourceManager::File::CommonInitializer(const FileSpec &file_spec,
+void SourceManager::File::CommonInitializer(SupportFileSP support_file_sp,
TargetSP target_sp) {
// Set the file and update the modification time.
- SetFileSpec(file_spec);
+ SetSupportFile(support_file_sp);
// Always update the source map modification ID if we have a target.
if (target_sp)
@@ -472,65 +479,76 @@ void SourceManager::File::CommonInitializer(const FileSpec &file_spec,
if (m_mod_time == llvm::sys::TimePoint<>()) {
if (target_sp) {
// If this is just a file name, try finding it in the target.
- if (!file_spec.GetDirectory() && file_spec.GetFilename()) {
- bool check_inlines = false;
- SymbolContextList sc_list;
- size_t num_matches =
- target_sp->GetImages().ResolveSymbolContextForFilePath(
- file_spec.GetFilename().AsCString(), 0, check_inlines,
- SymbolContextItem(eSymbolContextModule |
- eSymbolContextCompUnit),
- sc_list);
- bool got_multiple = false;
- if (num_matches != 0) {
- if (num_matches > 1) {
- CompileUnit *test_cu = nullptr;
- for (const SymbolContext &sc : sc_list) {
- if (sc.comp_unit) {
- if (test_cu) {
- if (test_cu != sc.comp_unit)
- got_multiple = true;
- break;
- } else
- test_cu = sc.comp_unit;
+ {
+ FileSpec file_spec = support_file_sp->GetSpecOnly();
+ if (!file_spec.GetDirectory() && file_spec.GetFilename()) {
+ bool check_inlines = false;
+ SymbolContextList sc_list;
+ size_t num_matches =
+ target_sp->GetImages().ResolveSymbolContextForFilePath(
+ file_spec.GetFilename().AsCString(), 0, check_inlines,
+ SymbolContextItem(eSymbolContextModule |
+ eSymbolContextCompUnit),
+ sc_list);
+ bool got_multiple = false;
+ if (num_matches != 0) {
+ if (num_matches > 1) {
+ CompileUnit *test_cu = nullptr;
+ for (const SymbolContext &sc : sc_list) {
+ if (sc.comp_unit) {
+ if (test_cu) {
+ if (test_cu != sc.comp_unit)
+ got_multiple = true;
+ break;
+ } else
+ test_cu = sc.comp_unit;
+ }
}
}
- }
- if (!got_multiple) {
- SymbolContext sc;
- sc_list.GetContextAtIndex(0, sc);
- if (sc.comp_unit)
- SetFileSpec(sc.comp_unit->GetPrimaryFile());
+ if (!got_multiple) {
+ SymbolContext sc;
+ sc_list.GetContextAtIndex(0, sc);
+ if (sc.comp_unit)
+ SetSupportFile(std::make_shared<SupportFile>(
+ sc.comp_unit->GetPrimaryFile()));
+ }
}
}
}
// Try remapping the file if it doesn't exist.
- if (!FileSystem::Instance().Exists(m_file_spec)) {
- // Check target specific source remappings (i.e., the
- // target.source-map setting), then fall back to the module
- // specific remapping (i.e., the .dSYM remapping dictionary).
- auto remapped = target_sp->GetSourcePathMap().FindFile(m_file_spec);
- if (!remapped) {
- FileSpec new_spec;
- if (target_sp->GetImages().FindSourceFile(m_file_spec, new_spec))
- remapped = new_spec;
+ {
+ FileSpec file_spec = support_file_sp->GetSpecOnly();
+ if (!FileSystem::Instance().Exists(file_spec)) {
+ // Check target specific source remappings (i.e., the
+ // target.source-map setting), then fall back to the module
+ // specific remapping (i.e., the .dSYM remapping dictionary).
+ auto remapped = target_sp->GetSourcePathMap().FindFile(file_spec);
+ if (!remapped) {
+ FileSpec new_spec;
+ if (target_sp->GetImages().FindSourceFile(file_spec, new_spec))
+ remapped = new_spec;
+ }
+ if (remapped)
+ SetSupportFile(std::make_shared<SupportFile>(
+ *remapped, support_file_sp->GetChecksum()));
}
- if (remapped)
- SetFileSpec(*remapped);
}
}
}
// If the file exists, read in the data.
if (m_mod_time != llvm::sys::TimePoint<>())
- m_data_sp = FileSystem::Instance().CreateDataBuffer(m_file_spec);
+ m_data_sp = FileSystem::Instance().CreateDataBuffer(
+ m_support_file_sp->GetSpecOnly());
}
-void SourceManager::File::SetFileSpec(FileSpec file_spec) {
+void SourceManager::File::SetSupportFile(lldb::SupportFileSP support_file_sp) {
+ FileSpec file_spec = support_file_sp->GetSpecOnly();
resolve_tilde(file_spec);
- m_file_spec = std::move(file_spec);
- m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
+ m_support_file_sp =
+ std::make_shared<SupportFile>(file_spec, support_file_sp->GetChecksum());
+ m_mod_time = FileSystem::Instance().GetModificationTime(file_spec);
}
uint32_t SourceManager::File::GetLineOffset(uint32_t line) {
@@ -603,7 +621,8 @@ bool SourceManager::File::ModificationTimeIsStale() const {
// TODO: use host API to sign up for file modifications to anything in our
// source cache and only update when we determine a file has been updated.
// For now we check each time we want to display info for the file.
- auto curr_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
+ auto curr_mod_time = FileSystem::Instance().GetModificationTime(
+ m_support_file_sp->GetSpecOnly());
return curr_mod_time != llvm::sys::TimePoint<>() &&
m_mod_time != curr_mod_time;
}
@@ -644,7 +663,8 @@ size_t SourceManager::File::DisplaySourceLines(uint32_t line,
debugger_sp->GetStopShowColumnAnsiSuffix());
HighlighterManager mgr;
- std::string path = GetFileSpec().GetPath(/*denormalize*/ false);
+ std::string path =
+ GetSupportFile()->GetSpecOnly().GetPath(/*denormalize*/ false);
// FIXME: Find a way to get the definitive language this file was written in
// and pass it to the highlighter.
const auto &h = mgr.getHighlighterFor(lldb::eLanguageTypeUnknown, path);
@@ -698,7 +718,8 @@ void SourceManager::File::FindLinesMatchingRegex(
bool lldb_private::operator==(const SourceManager::File &lhs,
const SourceManager::File &rhs) {
- if (lhs.m_file_spec != rhs.m_file_spec)
+ if (!lhs.GetSupportFile()->Equal(*rhs.GetSupportFile(),
+ SupportFile::eEqualChecksumIfSet))
return false;
return lhs.m_mod_time == rhs.m_mod_time;
}
@@ -778,9 +799,9 @@ void SourceManager::SourceFileCache::AddSourceFile(const FileSpec &file_spec,
assert(file_sp && "invalid FileSP");
AddSourceFileImpl(file_spec, file_sp);
- const FileSpec &resolved_file_spec = file_sp->GetFileSpec();
+ const FileSpec &resolved_file_spec = file_sp->GetSupportFile()->GetSpecOnly();
if (file_spec != resolved_file_spec)
- AddSourceFileImpl(file_sp->GetFileSpec(), file_sp);
+ AddSourceFileImpl(file_sp->GetSupportFile()->GetSpecOnly(), file_sp);
}
void SourceManager::SourceFileCache::RemoveSourceFile(const FileSP &file_sp) {
diff --git a/lldb/unittests/Core/SourceManagerTest.cpp b/lldb/unittests/Core/SourceManagerTest.cpp
index 58d6f6cb3f8503..26ab0edffb398d 100644
--- a/lldb/unittests/Core/SourceManagerTest.cpp
+++ b/lldb/unittests/Core/SourceManagerTest.cpp
@@ -8,6 +8,7 @@
#include "lldb/Core/SourceManager.h"
#include "lldb/Host/FileSystem.h"
+#include "lldb/Utility/SupportFile.h"
#include "gtest/gtest.h"
#include "TestingSupport/MockTildeExpressionResolver.h"
@@ -29,8 +30,8 @@ TEST_F(SourceFileCache, FindSourceFileFound) {
// Insert: foo
FileSpec foo_file_spec("foo");
- auto foo_file_sp =
- std::make_shared<SourceManager::File>(foo_file_spec, lldb::DebuggerSP());
+ auto foo_file_sp = std::make_shared<SourceManager::File>(
+ std::make_shared<SupportFile>(foo_file_spec), lldb::DebuggerSP());
cache.AddSourceFile(foo_file_spec, foo_file_sp);
// Query: foo, expect found.
@@ -43,8 +44,8 @@ TEST_F(SourceFileCache, FindSourceFileNotFound) {
// Insert: foo
FileSpec foo_file_spec("foo");
- auto foo_file_sp =
- std::make_shared<SourceManager::File>(foo_file_spec, lldb::DebuggerSP());
+ auto foo_file_sp = std::make_shared<SourceManager::File>(
+ std::make_shared<SupportFile>(foo_file_spec), lldb::DebuggerSP());
cache.AddSourceFile(foo_file_spec, foo_file_sp);
// Query: bar, expect not found.
@@ -63,7 +64,8 @@ TEST_F(SourceFileCache, FindSourceFileByUnresolvedPath) {
// Create the file with the resolved file spec.
auto foo_file_sp = std::make_shared<SourceManager::File>(
- resolved_foo_file_spec, lldb::DebuggerSP());
+ std::make_shared<SupportFile>(resolved_foo_file_spec),
+ lldb::DebuggerSP());
// Cache the result with the unresolved file spec.
cache.AddSourceFile(foo_file_spec, foo_file_sp);
|
There was a problem hiding this comment.
I was skeptical that a SupportFile would be the right choice, but it's documented as a:
Wraps either a FileSpec that represents a local file or a source file whose contents is known (for example because it can be reconstructed from debug info), but that hasn't been written to a file yet.
| // has source mappings, this might be different from | ||
| // m_file_spec_orig) | ||
| /// The original support file that was used. | ||
| lldb::SupportFileSP m_original_support_file_sp; |
There was a problem hiding this comment.
I suppose the SourceManager cannot own it because the SupportFile may outlive it?
|
|
||
| /// The actually support file being used. If the target | ||
| /// has source mappings, this might be different from | ||
| /// the original support file. |
There was a problem hiding this comment.
Is this yet another case where one support file may have different mappings in two targets at the same time?
(Asking whether this is the right ownership again)
There was a problem hiding this comment.
Looking at the code... yes.
adrian-prantl
approved these changes
Aug 29, 2024
|
|
||
| /// The actually support file being used. If the target | ||
| /// has source mappings, this might be different from | ||
| /// the original support file. |
There was a problem hiding this comment.
Looking at the code... yes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To support detecting MD5 checksum mismatches, store a SupportFile rather than a plain FileSpec in SourceManager::File.