Remove m_last_file_sp from SourceManager

[emrekultursay]

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.

[labath]

It's not clear to me what is the user-visible effect of this. It sounds like there should be one, but I don't know what it is...

lldb/source/Core/SourceManager.cpp

175–176

if(FileSP last_file_sp = GetLastFile())

253

Use GetLastFile, since we already have it.

267

same here

[emrekultursay]

In D76806#1943062, @labath wrote:

It's not clear to me what is the user-visible effect of this. It sounds like there should be one, but I don't know what it is...

If LLDB will cache FileSP shared pointers, there should be only one place for caching, and it's the Source File Cache, which can be enabled/disabled by the user.
User-facing effect of this change can be found in bug: llvm.org/PR45310

[emrekultursay]

Applied suggestions from labath@

[emrekultursay]

Added test that verifies end-to-end impact of
"setting set use_source_cache false" on Windows.

[emrekultursay]

Cleanup the Python test to make it a bit more readable

[emrekultursay]

PTAL.

[labath]

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

17 ↗

(On Diff #253260)

Could you remove this decorator? The test is not extremely interesting on non-windows hosts, but the flow should still work, so there's no harm in testing it.

22 ↗

(On Diff #253260)

technically, this should be hostoslist=no_match(...)

30–70 ↗

(On Diff #253260)

All of this could be done via a single lldbutil.run_to_source_breakpoint call (that's a fairly new thing, so it's possible the test you based this on is not using it yet).

40 ↗

(On Diff #253260)

Please avoid modifying the source tree. You can take a look at test/API/source-manager/Makefile to see how to build binaries with sources in the build tree.

86–87 ↗

(On Diff #253260)

maybe:

if is_cache_enabled:
  self.assertFalse(is_file_removed)

(and similarly for the other case).

lldb/test/API/commands/settings/use_source_cache/header.h

1 ↗

(On Diff #253260)

Does this need to be in a separate file? Could you just put it into main.cpp ?

[emrekultursay]

PTAL. Applied suggested changes, thanks for the review!

[emrekultursay]

Fix formatting.

[labath]

I think this is fine now. Jim, do you have any more thoughts on this patchset?

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

40 ↗

(On Diff #253651)

self.assertGreater

[emrekultursay]

use assertGreater instead of assertTrue(x>y)

[jingham]

There was one missing use of GetLastFile (as opposed to doing it by hand.)

Getting the default file is not used in any performance critical way that I'm aware of. It's mostly used for "break set -l" with no "-f" and list with no arguments and commands of that sort. So the extra lookup to get the FileSP from the source manager instead of caching it directly should not cause problems. And if you asked not to cache, you've already decided to pay the cost for that.

LGTM with that trivial fix.

lldb/source/Core/SourceManager.cpp

175–176

Doesn't look like this got converted to GetLastFile.

[emrekultursay]

Fix missing use of GetLastFile

[emrekultursay]

I don't have commit access, can someone with access commit all three approved CLs in this stack?

[labath]

I am afraid that the last patch in this series introduces a bunch of failures for me. The failures appear to be more-or-less consistent, but the impacted tests seem pretty random:

lldb-api :: commands/expression/persistent_ptr_update/TestPersistentPtrUpdate.py
lldb-api :: commands/expression/persistent_variables/TestPersistentVariables.py
lldb-api :: commands/watchpoints/hello_watchlocation/TestWatchLocation.py
lldb-api :: commands/watchpoints/hello_watchpoint/TestMyFirstWatchpoint.py
lldb-api :: commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py
lldb-api :: commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandLLDB.py
lldb-api :: commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py
lldb-api :: commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
lldb-api :: commands/watchpoints/watchpoint_set_command/TestWatchLocationWithWatchSet.py
lldb-api :: functionalities/breakpoint/auto_continue/TestBreakpointAutoContinue.py
lldb-api :: functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/atomic/TestLibCxxAtomic.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/function/TestLibCxxFunction.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/map/TestDataFormatterStdMap.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/tuple/TestDataFormatterStdTuple.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
lldb-api :: functionalities/data-formatter/data-formatter-stl/libstdcpp/vector/TestDataFormatterStdVector.py
lldb-api :: functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
lldb-api :: functionalities/signal/handle-abrt/TestHandleAbort.py
lldb-api :: functionalities/type_completion/TestTypeCompletion.py
lldb-api :: lang/c/register_variables/TestRegisterVariables.py
lldb-api :: lang/cpp/class_types/TestClassTypes.py
lldb-api :: python_api/sbvalue_persist/TestSBValuePersist.py
lldb-shell :: Driver/TestSingleQuote.test
lldb-shell :: Settings/TestFrameFormatMangling.test

Reverting the last patch makes the problem go away, but I am not sure the problem is really there. Given that this series exposes a bunch of previously completely unused code, it could be that the problem is completely elsewhere.

Anyway, this needs to be redone somehow.

[emrekultursay]

Fix broken tests.

[emrekultursay]

Thanks for noticing the test breakages Pavel. I fixed the bug, and verified that the tests you mentioned pass. PTAL.

[labath]

In D76806#1973035, @emrekultursay wrote:

Thanks for noticing the test breakages Pavel. I fixed the bug, and verified that the tests you mentioned pass. PTAL.

Everything looks fine now. Thanks for fixing that (it would be nice to mention what the bug was so one doesn't have to dig through the patch history to figure it out). I'm going to commit this shortly.

lldb/include/lldb/Core/SourceManager.h

public:
   // 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.
   SourceManager(const lldb::DebuggerSP &debugger_sp);
   SourceManager(const lldb::TargetSP &target_sp);
 
   ~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,
                                     uint32_t column, uint32_t context_before,
                                     uint32_t context_after,
                                     const char *current_line_cstr, Stream *s,
                                     const SymbolContextList *bp_locs = nullptr);
 
   // This variant uses the last file we visited.
   size_t DisplaySourceLinesWithLineNumbersUsingLastFile(
       uint32_t start_line, uint32_t count, uint32_t curr_line, uint32_t column,
       const char *current_line_cstr, Stream *s,
       const SymbolContextList *bp_locs = nullptr);
 
   size_t DisplayMoreWithLineNumbers(Stream *s, uint32_t count, bool reverse,
                                     const SymbolContextList *bp_locs = nullptr);
 
   bool SetDefaultFileAndLine(const FileSpec &file_spec, uint32_t line);
 
   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,
                               std::vector<uint32_t> &match_lines);
 
   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::TargetWP m_target_wp;
   lldb::DebuggerWP m_debugger_wp;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(SourceManager);
 };
 
 bool operator==(const SourceManager::File &lhs, const SourceManager::File &rhs);
 
 } // namespace lldb_private
 
 #endif // LLDB_CORE_SOURCEMANAGER_H

lldb/source/Core/SourceManager.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 
 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_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());
-
   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());
 
   // 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() !=
if (!m_default_set) {
     FileSpec tmp_spec;
     uint32_t tmp_line;
     GetDefaultFileAndLine(tmp_spec, tmp_line);
   }
 
   m_last_line = start_line;
   m_last_count = count;
 
-  if (m_last_file_sp.get()) {
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  if (last_file_sp.get()) {
     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;
       }
 
       char prefix[32] = "";
       if (bp_locs) {
         uint32_t bp_count = bp_locs->NumLineEntriesWithLine(line);
 
for (uint32_t line = start_line; line <= end_line; ++line) {
       // So far we treated column 0 as a special 'no column value', but
       // DisplaySourceLines starts counting columns from 0 (and no column is
       // expressed by passing an empty optional).
       llvm::Optional<size_t> columnToHighlight;
       if (line == curr_line && column)
         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)
           s->PutChar(src_line[i] == '\t' ? '\t' : ' ');
         // Now add the caret.
         s->Printf("^\n");
       }
       if (this_line_size == 0) {
size_t SourceManager::DisplaySourceLinesWithLineNumbers(
 
   uint32_t start_line;
   uint32_t count = context_before + context_after + 1;
   if (line > context_before)
     start_line = line - context_before;
   else
     start_line = 1;
 
-  if (m_last_file_sp.get() != file_sp.get()) {
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  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);
 }
 
 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(GetFile(m_last_file_spec));
+  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;
 
     if (reverse && m_last_line == 1)
       return 0;
 
     if (count > 0)
       m_last_count = count;
if (last_file_sp) {
     return DisplaySourceLinesWithLineNumbersUsingLastFile(
         m_last_line, m_last_count, UINT32_MAX, column, "", s, bp_locs);
   }
   return 0;
 }
 
 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();
+  FileSP last_file_sp(GetFile(m_last_file_spec));
+  if (last_file_sp) {
+    file_spec = last_file_sp->GetFileSpec();
     line = m_last_line;
     return true;
   } else if (!m_default_set) {
     TargetSP target_sp(m_target_wp.lock());
 
     if (target_sp) {
       // If nobody has set the default file and line then try here.  If there's
       // no executable, then we will try again later when there is one.
if (target_sp) {
           SymbolContext sc;
           sc_list.GetContextAtIndex(idx, sc);
           if (sc.function) {
             lldb_private::LineEntry line_entry;
             if (sc.function->GetAddressRange()
                     .GetBaseAddress()
                     .CalculateSymbolContextLineEntry(line_entry)) {
               SetDefaultFileAndLine(line_entry.file, line_entry.line);
-              file_spec = m_last_file_sp->GetFileSpec();
+              file_spec = last_file_sp->GetFileSpec();
               line = m_last_line;
               return true;
             }
           }
         }
       }
     }
   }