diff --git a/lldb/include/lldb/Core/SourceManager.h b/lldb/include/lldb/Core/SourceManager.h --- a/lldb/include/lldb/Core/SourceManager.h +++ b/lldb/include/lldb/Core/SourceManager.h @@ -14,6 +14,7 @@ #include "lldb/lldb-forward.h" #include "llvm/Support/Chrono.h" +#include "llvm/Support/RWMutex.h" #include #include @@ -36,11 +37,11 @@ const SourceManager::File &rhs); public: - File(const FileSpec &file_spec, Target *target); + File(const FileSpec &file_spec, lldb::TargetSP target_sp); File(const FileSpec &file_spec, lldb::DebuggerSP debugger_sp); - ~File() = default; - void UpdateIfNeeded(); + bool ModificationTimeIsStale() const; + bool PathRemappingIsStale() const; size_t DisplaySourceLines(uint32_t line, std::optional column, uint32_t context_before, uint32_t context_after, @@ -89,22 +90,29 @@ typedef std::vector LineOffsets; LineOffsets m_offsets; lldb::DebuggerWP m_debugger_wp; + lldb::TargetWP m_target_wp; private: - void CommonInitializer(const FileSpec &file_spec, Target *target); + void CommonInitializer(const FileSpec &file_spec, lldb::TargetSP target_sp); }; typedef std::shared_ptr FileSP; - // The SourceFileCache class separates the source manager from the cache of - // source files, so the cache can be stored in the Debugger, but the source - // managers can be per target. + /// The SourceFileCache class separates the source manager from the cache of + /// source files. There is one source manager per Target but both the Debugger + /// and the Process have their own source caches. + /// + /// The SourceFileCache just handles adding, storing, removing and looking up + /// source files. The caching policies are implemented in + /// SourceManager::GetFile. class SourceFileCache { public: SourceFileCache() = default; ~SourceFileCache() = default; void AddSourceFile(const FileSpec &file_spec, FileSP file_sp); + void RemoveSourceFile(const FileSP &file_sp); + FileSP FindSourceFile(const FileSpec &file_spec) const; // Removes all elements from the cache. @@ -117,15 +125,17 @@ typedef std::map FileCache; FileCache m_file_cache; + + mutable llvm::sys::RWMutex m_mutex; }; - // Constructors and Destructors - // A source manager can be made with a non-null target, in which case it can - // use the path remappings to find - // source files that are not in their build locations. With no target it - // won't be able to do this. + /// A source manager can be made with a valid Target, in which case it can use + /// the path remappings to find source files that are not in their build + /// locations. Without a target it won't be able to do this. + /// @{ SourceManager(const lldb::DebuggerSP &debugger_sp); SourceManager(const lldb::TargetSP &target_sp); + /// @} ~SourceManager(); diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -25,6 +25,7 @@ #include "lldb/Breakpoint/BreakpointSiteList.h" #include "lldb/Core/LoadedModuleInfoList.h" #include "lldb/Core/PluginInterface.h" +#include "lldb/Core/SourceManager.h" #include "lldb/Core/ThreadSafeValue.h" #include "lldb/Core/ThreadedCommunication.h" #include "lldb/Core/UserSettingsController.h" @@ -2573,6 +2574,10 @@ virtual void ForceScriptedState(lldb::StateType state) {} + SourceManager::SourceFileCache &GetSourceFileCache() { + return m_source_file_cache; + } + protected: friend class Trace; @@ -3043,6 +3048,9 @@ std::unique_ptr m_dlopen_utility_func_up; llvm::once_flag m_dlopen_utility_func_flag_once; + /// Per process source file cache. + SourceManager::SourceFileCache m_source_file_cache; + size_t RemoveBreakpointOpcodesFromBuffer(lldb::addr_t addr, size_t size, uint8_t *buf) const; diff --git a/lldb/include/lldb/Utility/LLDBLog.h b/lldb/include/lldb/Utility/LLDBLog.h --- a/lldb/include/lldb/Utility/LLDBLog.h +++ b/lldb/include/lldb/Utility/LLDBLog.h @@ -48,6 +48,7 @@ Unwind = Log::ChannelFlag<29>, Watchpoints = Log::ChannelFlag<30>, OnDemand = Log::ChannelFlag<31>, + Source = Log::ChannelFlag<32>, LLVM_MARK_AS_BITMASK_ENUM(OnDemand), }; diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp --- a/lldb/source/Commands/CommandObjectSource.cpp +++ b/lldb/source/Commands/CommandObjectSource.cpp @@ -22,6 +22,7 @@ #include "lldb/Symbol/CompileUnit.h" #include "lldb/Symbol/Function.h" #include "lldb/Symbol/Symbol.h" +#include "lldb/Target/Process.h" #include "lldb/Target/SectionLoadList.h" #include "lldb/Target/StackFrame.h" #include "lldb/Utility/FileSpec.h" @@ -1213,8 +1214,18 @@ protected: bool DoExecute(Args &command, CommandReturnObject &result) override { + // Dump the debugger source cache. + result.GetOutputStream() << "Debugger Source File Cache\n"; SourceManager::SourceFileCache &cache = GetDebugger().GetSourceFileCache(); cache.Dump(result.GetOutputStream()); + + // Dump the process source cache. + if (ProcessSP process_sp = m_exe_ctx.GetProcessSP()) { + result.GetOutputStream() << "\nProcess Source File Cache\n"; + SourceManager::SourceFileCache &cache = process_sp->GetSourceFileCache(); + cache.Dump(result.GetOutputStream()); + } + result.SetStatus(eReturnStatusSuccessFinishResult); return result.Succeeded(); } @@ -1230,8 +1241,14 @@ protected: bool DoExecute(Args &command, CommandReturnObject &result) override { + // Clear the debugger cache. SourceManager::SourceFileCache &cache = GetDebugger().GetSourceFileCache(); cache.Clear(); + + // Clear the process cache. + if (ProcessSP process_sp = m_exe_ctx.GetProcessSP()) + process_sp->GetSourceFileCache().Clear(); + result.SetStatus(eReturnStatusSuccessFinishNoResult); return result.Succeeded(); } diff --git a/lldb/source/Core/SourceManager.cpp b/lldb/source/Core/SourceManager.cpp --- a/lldb/source/Core/SourceManager.cpp +++ b/lldb/source/Core/SourceManager.cpp @@ -21,10 +21,13 @@ #include "lldb/Symbol/LineEntry.h" #include "lldb/Symbol/SymbolContext.h" #include "lldb/Target/PathMappingList.h" +#include "lldb/Target/Process.h" #include "lldb/Target/Target.h" #include "lldb/Utility/AnsiTerminal.h" #include "lldb/Utility/ConstString.h" #include "lldb/Utility/DataBuffer.h" +#include "lldb/Utility/LLDBLog.h" +#include "lldb/Utility/Log.h" #include "lldb/Utility/RegularExpression.h" #include "lldb/Utility/Stream.h" #include "lldb/lldb-enumerations.h" @@ -73,36 +76,89 @@ SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) { if (!file_spec) - return nullptr; + return {}; - DebuggerSP debugger_sp(m_debugger_wp.lock()); - FileSP file_sp; - if (debugger_sp && debugger_sp->GetUseSourceCache()) - file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec); + Log *log = GetLog(LLDBLog::Source); + DebuggerSP debugger_sp(m_debugger_wp.lock()); TargetSP target_sp(m_target_wp.lock()); - // It the target source path map has been updated, get this file again so we - // can successfully remap the source file - if (target_sp && file_sp && - file_sp->GetSourceMapModificationID() != - target_sp->GetSourcePathMap().GetModificationID()) - file_sp.reset(); + if (!debugger_sp || !debugger_sp->GetUseSourceCache()) { + LLDB_LOG(log, "Source file caching disabled: creating new source file: {0}", + file_spec); + if (target_sp) + return std::make_shared(file_spec, target_sp); + return std::make_shared(file_spec, debugger_sp); + } + + ProcessSP process_sp = target_sp ? target_sp->GetProcessSP() : ProcessSP(); + + // Check the process source cache first. This is the fast path which avoids + // touching the file system unless the path remapping has changed. + if (process_sp) { + if (FileSP file_sp = + process_sp->GetSourceFileCache().FindSourceFile(file_spec)) { + LLDB_LOG(log, "Found source file in the process cache: {0}", file_spec); + if (file_sp->PathRemappingIsStale()) { + LLDB_LOG(log, "Path remapping is stale: removing file from caches: {0}", + file_spec); + + // Remove the file from the debugger and process cache. Otherwise we'll + // hit the same issue again below when querying the debugger cache. + debugger_sp->GetSourceFileCache().RemoveSourceFile(file_sp); + process_sp->GetSourceFileCache().RemoveSourceFile(file_sp); + + file_sp.reset(); + } else { + return file_sp; + } + } + } - // Update the file contents if needed if we found a file + // Cache miss in the process cache. Check the debugger source cache. + FileSP file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec); + + // We found the file in the debugger cache. Check if anything invalidated our + // cache result. if (file_sp) - file_sp->UpdateIfNeeded(); + LLDB_LOG(log, "Found source file in the debugger cache: {0}", file_spec); + + // Check if the path remapping has changed. + if (file_sp && file_sp->PathRemappingIsStale()) { + LLDB_LOG(log, "Path remapping is stale: {0}", file_spec); + file_sp.reset(); + } + + // Check if the modification time has changed. + if (file_sp && file_sp->ModificationTimeIsStale()) { + LLDB_LOG(log, "Modification time is stale: {0}", file_spec); + file_sp.reset(); + } + + // Check if the file exists on disk. + if (file_sp && !FileSystem::Instance().Exists(file_sp->GetFileSpec())) { + LLDB_LOG(log, "File doesn't exist on disk: {0}", file_spec); + file_sp.reset(); + } - // If file_sp is no good or it points to a non-existent file, reset it. - if (!file_sp || !FileSystem::Instance().Exists(file_sp->GetFileSpec())) { + // If at this point we don't have a valid file, it means we either didn't find + // it in the debugger cache or something caused it to be invalidated. + if (!file_sp) { + LLDB_LOG(log, "Creating and caching new source file: {0}", file_spec); + + // (Re)create the file. if (target_sp) - file_sp = std::make_shared(file_spec, target_sp.get()); + file_sp = std::make_shared(file_spec, target_sp); else file_sp = std::make_shared(file_spec, debugger_sp); - if (debugger_sp && debugger_sp->GetUseSourceCache()) - debugger_sp->GetSourceFileCache().AddSourceFile(file_spec, file_sp); + // Add the file to the debugger and process cache. If the file was + // invalidated, this will overwrite it. + debugger_sp->GetSourceFileCache().AddSourceFile(file_spec, file_sp); + if (process_sp) + process_sp->GetSourceFileCache().AddSourceFile(file_spec, file_sp); } + return file_sp; } @@ -391,33 +447,36 @@ SourceManager::File::File(const FileSpec &file_spec, lldb::DebuggerSP debugger_sp) : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(), - m_debugger_wp(debugger_sp) { - CommonInitializer(file_spec, nullptr); + m_debugger_wp(debugger_sp), m_target_wp(TargetSP()) { + CommonInitializer(file_spec, {}); } -SourceManager::File::File(const FileSpec &file_spec, Target *target) +SourceManager::File::File(const FileSpec &file_spec, TargetSP target_sp) : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(), - m_debugger_wp(target ? target->GetDebugger().shared_from_this() - : DebuggerSP()) { - CommonInitializer(file_spec, target); + m_debugger_wp(target_sp ? target_sp->GetDebugger().shared_from_this() + : DebuggerSP()), + m_target_wp(target_sp) { + CommonInitializer(file_spec, target_sp); } void SourceManager::File::CommonInitializer(const FileSpec &file_spec, - Target *target) { + TargetSP target_sp) { // Set the file and update the modification time. SetFileSpec(file_spec); + // Always update the source map modification ID if we have a target. + if (target_sp) + m_source_map_mod_id = target_sp->GetSourcePathMap().GetModificationID(); + // File doesn't exist. if (m_mod_time == llvm::sys::TimePoint<>()) { - if (target) { - m_source_map_mod_id = target->GetSourcePathMap().GetModificationID(); - + 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->GetImages().ResolveSymbolContextForFilePath( + target_sp->GetImages().ResolveSymbolContextForFilePath( file_spec.GetFilename().AsCString(), 0, check_inlines, SymbolContextItem(eSymbolContextModule | eSymbolContextCompUnit), @@ -451,10 +510,10 @@ // 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->GetSourcePathMap().FindFile(m_file_spec); + auto remapped = target_sp->GetSourcePathMap().FindFile(m_file_spec); if (!remapped) { FileSpec new_spec; - if (target->GetImages().FindSourceFile(m_file_spec, new_spec)) + if (target_sp->GetImages().FindSourceFile(m_file_spec, new_spec)) remapped = new_spec; } if (remapped) @@ -540,18 +599,20 @@ return false; } -void SourceManager::File::UpdateIfNeeded() { +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); + return curr_mod_time != llvm::sys::TimePoint<>() && + m_mod_time != curr_mod_time; +} - if (curr_mod_time != llvm::sys::TimePoint<>() && - m_mod_time != curr_mod_time) { - m_mod_time = curr_mod_time; - m_data_sp = FileSystem::Instance().CreateDataBuffer(m_file_spec); - m_offsets.clear(); - } +bool SourceManager::File::PathRemappingIsStale() const { + if (TargetSP target_sp = m_target_wp.lock()) + return GetSourceMapModificationID() != + target_sp->GetSourcePathMap().GetModificationID(); + return false; } size_t SourceManager::File::DisplaySourceLines(uint32_t line, @@ -712,6 +773,8 @@ void SourceManager::SourceFileCache::AddSourceFile(const FileSpec &file_spec, FileSP file_sp) { + llvm::sys::ScopedWriter guard(m_mutex); + assert(file_sp && "invalid FileSP"); AddSourceFileImpl(file_spec, file_sp); @@ -720,6 +783,22 @@ AddSourceFileImpl(file_sp->GetFileSpec(), file_sp); } +void SourceManager::SourceFileCache::RemoveSourceFile(const FileSP &file_sp) { + llvm::sys::ScopedWriter guard(m_mutex); + + assert(file_sp && "invalid FileSP"); + + // Iterate over all the elements in the cache. + // This is expensive but a relatively uncommon operation. + auto it = m_file_cache.begin(); + while (it != m_file_cache.end()) { + if (it->second == file_sp) + it = m_file_cache.erase(it); + else + it++; + } +} + void SourceManager::SourceFileCache::AddSourceFileImpl( const FileSpec &file_spec, FileSP file_sp) { FileCache::iterator pos = m_file_cache.find(file_spec); @@ -733,11 +812,12 @@ SourceManager::FileSP SourceManager::SourceFileCache::FindSourceFile( const FileSpec &file_spec) const { - FileSP file_sp; + llvm::sys::ScopedReader guard(m_mutex); + FileCache::const_iterator pos = m_file_cache.find(file_spec); if (pos != m_file_cache.end()) - file_sp = pos->second; - return file_sp; + return pos->second; + return {}; } void SourceManager::SourceFileCache::Dump(Stream &stream) const { diff --git a/lldb/source/Utility/LLDBLog.cpp b/lldb/source/Utility/LLDBLog.cpp --- a/lldb/source/Utility/LLDBLog.cpp +++ b/lldb/source/Utility/LLDBLog.cpp @@ -63,6 +63,7 @@ {{"on-demand"}, {"log symbol on-demand related activities"}, LLDBLog::OnDemand}, + {{"source"}, {"log source related activities"}, LLDBLog::Source}, }; static Log::Channel g_log_channel(g_categories, diff --git a/lldb/test/API/source-manager/TestSourceManager.py b/lldb/test/API/source-manager/TestSourceManager.py --- a/lldb/test/API/source-manager/TestSourceManager.py +++ b/lldb/test/API/source-manager/TestSourceManager.py @@ -10,6 +10,7 @@ """ import os +import io import stat import lldb @@ -37,6 +38,33 @@ self.file = self.getBuildArtifact("main-copy.c") self.line = line_number("main.c", "// Set break point at this line.") + def modify_content(self): + + # Read the main.c file content. + with io.open(self.file, "r", newline="\n") as f: + original_content = f.read() + if self.TraceOn(): + print("original content:", original_content) + + # Modify the in-memory copy of the original source code. + new_content = original_content.replace("Hello world", "Hello lldb", 1) + + # Modify the source code file. + # If the source was read only, the copy will also be read only. + # Run "chmod u+w" on it first so we can modify it. + statinfo = os.stat(self.file) + os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR) + + with io.open(self.file, "w", newline="\n") as f: + time.sleep(1) + f.write(new_content) + if self.TraceOn(): + print("new content:", new_content) + print( + "os.path.getmtime() after writing new content:", + os.path.getmtime(self.file), + ) + def get_expected_stop_column_number(self): """Return the 1-based column number of the first non-whitespace character in the breakpoint source line.""" @@ -234,32 +262,20 @@ self.fail("Fail to display source level breakpoints") self.assertTrue(int(m.group(1)) > 0) - # Read the main.c file content. - with io.open(self.file, "r", newline="\n") as f: - original_content = f.read() - if self.TraceOn(): - print("original content:", original_content) - - # Modify the in-memory copy of the original source code. - new_content = original_content.replace("Hello world", "Hello lldb", 1) + # Modify content + self.modify_content() - # Modify the source code file. - # If the source was read only, the copy will also be read only. - # Run "chmod u+w" on it first so we can modify it. - statinfo = os.stat(self.file) - os.chmod(self.file, statinfo.st_mode | stat.S_IWUSR) + # Display the source code again. We should not see the updated line. + self.expect( + "source list -f main-copy.c -l %d" % self.line, + SOURCE_DISPLAYED_CORRECTLY, + substrs=["Hello world"], + ) - with io.open(self.file, "w", newline="\n") as f: - time.sleep(1) - f.write(new_content) - if self.TraceOn(): - print("new content:", new_content) - print( - "os.path.getmtime() after writing new content:", - os.path.getmtime(self.file), - ) + # clear the source cache. + self.runCmd("source cache clear") - # Display the source code again. We should see the updated line. + # Display the source code again. Now we should see the updated line. self.expect( "source list -f main-copy.c -l %d" % self.line, SOURCE_DISPLAYED_CORRECTLY, @@ -338,3 +354,45 @@ # Make sure the main source file is no longer in the source cache. self.expect("source cache dump", matching=False, substrs=[self.file]) + + def test_source_cache_interactions(self): + self.build() + exe = self.getBuildArtifact("a.out") + + # Create a first target. + self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) + lldbutil.run_break_set_by_symbol( + self, "main", num_expected_locations=1 + ) + self.expect("run", RUN_SUCCEEDED, substrs=["Hello world"]) + + # Create a second target. + self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) + lldbutil.run_break_set_by_symbol( + self, "main", num_expected_locations=1 + ) + self.expect("run", RUN_SUCCEEDED, substrs=["Hello world"]) + + # Modify the source file content. + self.modify_content() + + # Clear the source cache. This will wipe the debugger and the process + # cache for the second process. + self.runCmd("source cache clear") + + # Make sure we're seeing the new content from the clean process cache. + self.expect("next", + SOURCE_DISPLAYED_CORRECTLY, + substrs=["Hello lldb"], + ) + + # Switch back to the first target. + self.runCmd("target select 0") + + # Make sure we're seeing the old content from the first target's + # process cache. + self.expect("next", + SOURCE_DISPLAYED_CORRECTLY, + substrs=["Hello world"], + ) + diff --git a/lldb/unittests/Core/SourceManagerTest.cpp b/lldb/unittests/Core/SourceManagerTest.cpp --- a/lldb/unittests/Core/SourceManagerTest.cpp +++ b/lldb/unittests/Core/SourceManagerTest.cpp @@ -30,7 +30,7 @@ // Insert: foo FileSpec foo_file_spec("foo"); auto foo_file_sp = - std::make_shared(foo_file_spec, nullptr); + std::make_shared(foo_file_spec, lldb::DebuggerSP()); cache.AddSourceFile(foo_file_spec, foo_file_sp); // Query: foo, expect found. @@ -44,7 +44,7 @@ // Insert: foo FileSpec foo_file_spec("foo"); auto foo_file_sp = - std::make_shared(foo_file_spec, nullptr); + std::make_shared(foo_file_spec, lldb::DebuggerSP()); cache.AddSourceFile(foo_file_spec, foo_file_sp); // Query: bar, expect not found. @@ -62,8 +62,8 @@ FileSystem::Instance().Resolve(resolved_foo_file_spec); // Create the file with the resolved file spec. - auto foo_file_sp = - std::make_shared(resolved_foo_file_spec, nullptr); + auto foo_file_sp = std::make_shared( + resolved_foo_file_spec, lldb::DebuggerSP()); // Cache the result with the unresolved file spec. cache.AddSourceFile(foo_file_spec, foo_file_sp);