[lldb] Remove m_last_file_sp from SourceManager

Summary:
...and replace it with m_last_file_spec instead.

When Source Cache is enabled, the value stored in m_last_file_sp is
already in the Source Cache, and caching it again in SourceManager
brings no extra benefit. All we need is to "remember" the last used
file, and FileSpec can serve the same purpose.

When Source Cache is disabled, the user explicitly requested no caching
of source files, and therefore, m_last_file_sp should NOT be used.

Bug: llvm.org/PR45310

Depends on D76805.

Reviewers: labath, jingham

Reviewed By: jingham

Subscribers: labath, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D76806

lldb/include/lldb/Core/SourceManager.h

@@ -119,7 +119,7 @@ class SourceManager {
 
   ~SourceManager();
 
-  FileSP GetLastFile() { return m_last_file_sp; }
+  FileSP GetLastFile() { return GetFile(m_last_file_spec); }
 
   size_t
   DisplaySourceLinesWithLineNumbers(const FileSpec &file, uint32_t line,
@@ -141,7 +141,9 @@ class SourceManager {
 
   bool GetDefaultFileAndLine(FileSpec &file_spec, uint32_t &line);
 
-  bool DefaultFileAndLineSet() { return (m_last_file_sp.get() != nullptr); }
+  bool DefaultFileAndLineSet() {
+    return (GetFile(m_last_file_spec).get() != nullptr);
+  }
 
   void FindLinesMatchingRegex(FileSpec &file_spec, RegularExpression &regex,
                               uint32_t start_line, uint32_t end_line,
@@ -150,7 +152,7 @@ class SourceManager {
   FileSP GetFile(const FileSpec &file_spec);
 
 protected:
-  FileSP m_last_file_sp;
+  FileSpec m_last_file_spec;
   uint32_t m_last_line;
   uint32_t m_last_count;
   bool m_default_set;

lldb/source/Core/SourceManager.cpp

@@ -52,27 +52,24 @@ static inline bool is_newline_char(char ch) { return ch == '\n' || ch == '\r'; }
 
 // SourceManager constructor
 SourceManager::SourceManager(const TargetSP &target_sp)
-    : m_last_file_sp(), m_last_line(0), m_last_count(0), m_default_set(false),
+    : m_last_line(0), m_last_count(0), m_default_set(false),
       m_target_wp(target_sp),
       m_debugger_wp(target_sp->GetDebugger().shared_from_this()) {}
 
 SourceManager::SourceManager(const DebuggerSP &debugger_sp)
-    : m_last_file_sp(), m_last_line(0), m_last_count(0), m_default_set(false),
-      m_target_wp(), m_debugger_wp(debugger_sp) {}
+    : m_last_line(0), m_last_count(0), m_default_set(false), m_target_wp(),
+      m_debugger_wp(debugger_sp) {}
 
 // Destructor
 SourceManager::~SourceManager() {}
 
 SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
-  bool same_as_previous =
-      m_last_file_sp &&
-      FileSpec::Match(file_spec, m_last_file_sp->GetFileSpec());
+  if (!file_spec)
+    return nullptr;
 
   DebuggerSP debugger_sp(m_debugger_wp.lock());
   FileSP file_sp;
-  if (same_as_previous)
-    file_sp = m_last_file_sp;
-  else if (debugger_sp && debugger_sp->GetUseSourceCache())
+  if (debugger_sp && debugger_sp->GetUseSourceCache())
     file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
 
   TargetSP target_sp(m_target_wp.lock());
@@ -178,10 +175,10 @@ size_t SourceManager::DisplaySourceLinesWithLineNumbersUsingLastFile(
   m_last_line = start_line;
   m_last_count = count;
 
-  if (m_last_file_sp.get()) {
+  if (FileSP last_file_sp = GetLastFile()) {
     const uint32_t end_line = start_line + count - 1;
     for (uint32_t line = start_line; line <= end_line; ++line) {
-      if (!m_last_file_sp->LineIsValid(line)) {
+      if (!last_file_sp->LineIsValid(line)) {
         m_last_line = UINT32_MAX;
         break;
       }
@@ -219,12 +216,12 @@ size_t SourceManager::DisplaySourceLinesWithLineNumbersUsingLastFile(
         columnToHighlight = column - 1;
 
       size_t this_line_size =
-          m_last_file_sp->DisplaySourceLines(line, columnToHighlight, 0, 0, s);
+          last_file_sp->DisplaySourceLines(line, columnToHighlight, 0, 0, s);
       if (column != 0 && line == curr_line &&
           should_show_stop_column_with_caret(debugger_sp)) {
         // Display caret cursor.
         std::string src_line;
-        m_last_file_sp->GetLine(line, src_line);
+        last_file_sp->GetLine(line, src_line);
         s->Printf("    \t");
         // Insert a space for every non-tab character in the source line.
         for (size_t i = 0; i + 1 < column && i < src_line.length(); ++i)
@@ -255,10 +252,11 @@ size_t SourceManager::DisplaySourceLinesWithLineNumbers(
   else
     start_line = 1;
 
-  if (m_last_file_sp.get() != file_sp.get()) {
+  FileSP last_file_sp(GetLastFile());
+  if (last_file_sp.get() != file_sp.get()) {
     if (line == 0)
       m_last_line = 0;
-    m_last_file_sp = file_sp;
+    m_last_file_spec = file_spec;
   }
   return DisplaySourceLinesWithLineNumbersUsingLastFile(
       start_line, count, line, column, current_line_cstr, s, bp_locs);
@@ -268,14 +266,15 @@ size_t SourceManager::DisplayMoreWithLineNumbers(
     Stream *s, uint32_t count, bool reverse, const SymbolContextList *bp_locs) {
   // If we get called before anybody has set a default file and line, then try
   // to figure it out here.
-  const bool have_default_file_line = m_last_file_sp && m_last_line > 0;
+  FileSP last_file_sp(GetLastFile());
+  const bool have_default_file_line = last_file_sp && m_last_line > 0;
   if (!m_default_set) {
     FileSpec tmp_spec;
     uint32_t tmp_line;
     GetDefaultFileAndLine(tmp_spec, tmp_line);
   }
 
-  if (m_last_file_sp) {
+  if (last_file_sp) {
     if (m_last_line == UINT32_MAX)
       return 0;
 
@@ -310,22 +309,21 @@ size_t SourceManager::DisplayMoreWithLineNumbers(
 
 bool SourceManager::SetDefaultFileAndLine(const FileSpec &file_spec,
                                           uint32_t line) {
-  FileSP old_file_sp = m_last_file_sp;
-  m_last_file_sp = GetFile(file_spec);
-
   m_default_set = true;
-  if (m_last_file_sp) {
+  FileSP file_sp(GetFile(file_spec));
+
+  if (file_sp) {
     m_last_line = line;
+    m_last_file_spec = file_spec;
     return true;
   } else {
-    m_last_file_sp = old_file_sp;
     return false;
   }
 }
 
 bool SourceManager::GetDefaultFileAndLine(FileSpec &file_spec, uint32_t &line) {
-  if (m_last_file_sp) {
-    file_spec = m_last_file_sp->GetFileSpec();
+  if (FileSP last_file_sp = GetLastFile()) {
+    file_spec = m_last_file_spec;
     line = m_last_line;
     return true;
   } else if (!m_default_set) {
@@ -355,7 +353,7 @@ bool SourceManager::GetDefaultFileAndLine(FileSpec &file_spec, uint32_t &line) {
                     .GetBaseAddress()
                     .CalculateSymbolContextLineEntry(line_entry)) {
               SetDefaultFileAndLine(line_entry.file, line_entry.line);
-              file_spec = m_last_file_sp->GetFileSpec();
+              file_spec = m_last_file_spec;
               line = m_last_line;
               return true;
             }

lldb/test/API/commands/settings/use_source_cache/Makefile

@@ -0,0 +1,8 @@
+CXX_SOURCES := main-copy.cpp
+
+include Makefile.rules
+
+
+# Copy file into the build folder to enable the test to modify it.
+main-copy.cpp: main.cpp
+	cp -f $< $@

lldb/test/API/commands/settings/use_source_cache/TestUseSourceCache.py

@@ -0,0 +1,69 @@
+"""
+Tests large source files are not locked on Windows when source cache is disabled
+"""
+
+import lldb
+import os
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from shutil import copy
+
+class SettingsUseSourceCacheTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_set_use_source_cache_false(self):
+        """Test that after 'set use-source-cache false', files are not locked."""
+        self.set_use_source_cache_and_test(False)
+
+    @skipIf(hostoslist=no_match(["windows"]))
+    def test_set_use_source_cache_true(self):
+        """Test that after 'set use-source-cache false', files are locked."""
+        self.set_use_source_cache_and_test(True)
+
+    def set_use_source_cache_and_test(self, is_cache_enabled):
+        """Common test for both True/False values of use-source-cache."""
+        self.build()
+
+        # Enable/Disable source cache
+        self.runCmd(
+                "settings set use-source-cache " +
+                ("true" if is_cache_enabled else "false"))
+
+        # Get paths for the main source file.
+        src = self.getBuildArtifact("main-copy.cpp")
+        self.assertTrue(src)
+
+        # Make sure source file is bigger than 16K to trigger memory mapping
+        self.assertGreater(os.stat(src).st_size, 4*4096)
+
+        target, process, thread, breakpoint = lldbutil.run_to_name_breakpoint(
+                self,
+                "calc")
+
+        # Show the source file contents to make sure LLDB loads src file.
+        self.runCmd("source list")
+
+        # Try deleting the source file.
+        is_file_removed = self.removeFile(src)
+
+        if is_cache_enabled:
+            self.assertFalse(
+                is_file_removed,
+                "Source cache is enabled, but delete file succeeded")
+
+        if not is_cache_enabled:
+            self.assertTrue(
+                is_file_removed,
+                "Source cache is disabled, but delete file failed")
+
+    def removeFile(self, src):
+        """Remove file and return true iff file was successfully removed."""
+        try:
+            os.remove(src)
+            return True
+        except Exception:
+            return False
+