Fix out of sync source code/executable when debugging

[n1tram1]

Summary

Currently lldb's SourceManager will check a source file for any updates each time it accesses it.
The issue is that if you are debugging an executable and you modify one of its source files, lldb will show you the updated source file BUT the executable hasn't changed.
So you end up stepping through lines that aren't what is being executed.

This patch prevents this being doing an early return in the case when the source is newer than the executable (thus not allowing the rest of the function to update the source file).

For now I feel like this is a very ugly patch but I can't find any other way to de-reference so many pointers safely.

This breaks test "test_modify_source_file_while_debugging (TestSourceManager.SourceManagerTestCase)" but as I explained above I don't think this is normal behavior. I discussed it with @kwk on IRC and he agreed.
Should I rewrite the test for the new expected behavior or was I wrong this whole time ?

[n1tram1]

Apply clang-format

[jankratochvil]

lldb/source/Core/SourceManager.cpp

560

if (!debugger_sp)
  return false;

562

if (!target_sp)
  return false;

563

One should find the Module matching current frame. As this will not work for debugging shared libraries.

[n1tram1]

Check the modification time against the current selected module.

(Previous patch was only checking against the current target's executable/main module)

[n1tram1]

Fixed in latest patch

[labath]

This looks like a bad upload. Each time you update the patch it should include the full list of changes, not just your recent additions.

[n1tram1]

Fix the latest patch.

(I hope I got it right this time, sorry it's my first time doing this)

[jingham]

I can see the cases where this would help, but the way you are doing it could lead to some odd behavior that might be confusing.

Suppose I have two shared libraries, libBar.dylib and libNotBar.dylib in my project. I debug the process, find a bug in FileFromNotBar.c. So I change FileFromNotBar.c and then rebuild libNotBar.dylib. Then I go back to lldb and hit "run" again.

Suppose the next thing to happen is that I hit a breakpoint in foo.c in libBar.dylib. If I'm filled with doubt about the change I made in FileNotFromBar.c, so I then do:

(lldb) source list -f FileFromNotBar.c -l 10

So you check the currently selected frame's module - which is libbar.dylib and find that it was built before FileFromNotBar.c and would show me the old version.

Showing me the latest version of the file is not great, but totally explicable. Whereas this error, when it happens, would be confusing and hard to understand.

[n1tram1]

In D78421#1993030, @jingham wrote:

So you check the currently selected frame's module - which is libbar.dylib and find that it was built before FileFromNotBar.c and would show me the old version.

Showing me the latest version of the file is not great, but totally explicable. Whereas this error, when it happens, would be confusing and hard to understand.

So instead I should try to find which module the source file belongs to and check that module's modification time (instead of getting the currently selected frame) ?

[n1tram1]

Have the File first check if it is the current stack frame being displayed and check if it needs an update that way.
Else, iteratively look for it's belonging module and check if it needs an update.

Also fix the SourceCache::AddSourceFile function by using the input file as a key (Before the key was always the default constructor of FileSpec).

[jankratochvil]

lldb/source/Core/SourceManager.cpp

579

Please do not consider this as some requirement for LLDB project.
But I do not like much this guessing what module it really belongs to.
Couldn't we save the Module where m_file_spec came from? For example DisplaySourceLinesWithLineNumbers would then have another Module & parameter that it would save with m_last_file_spec. Then ModuleList::FindSourceFile could be replaced+simplified.
Another possibility would be to return Module & from ModuleList::FindSourceFile which is already being used in SourceManager.cpp.

[n1tram1]

Passing a simple Module & sounds like an easy but I would also have to make SourceManager::GetFile take a Module &.

There is one corner case, let's say file.c is being used by Module A and Module B (which are both shared libraries). If file.c is modified and Module B recompile and reloaded, well will want to see the updated file.c when stepping through Module B but we will want the old file.c when stepping through Module B.
My issue is that the SourceCacheManager use only the FileSpec as a key to the cache, therefore it might fetch the wrong File (gets the file.c corresponding to Module A when we wanted the file.c corresponding to Module B).
For the same reason I can't just modify ModuleList::FindSourceFile to have it return a ModuleSP because it might either Module A or Module B.

I feel like there is a lot to be done for such a simple bug. I don't know it it's worth it...

[labath]

In D78421#2054544, @n1tram1 wrote:

Passing a simple Module & sounds like an easy but I would also have to make SourceManager::GetFile take a Module &.

There is one corner case, let's say file.c is being used by Module A and Module B (which are both shared libraries). If file.c is modified and Module B recompile and reloaded, well will want to see the updated file.c when stepping through Module B but we will want the old file.c when stepping through Module B.
My issue is that the SourceCacheManager use only the FileSpec as a key to the cache, therefore it might fetch the wrong File (gets the file.c corresponding to Module A when we wanted the file.c corresponding to Module B).
For the same reason I can't just modify ModuleList::FindSourceFile to have it return a ModuleSP because it might either Module A or Module B.

I feel like there is a lot to be done for such a simple bug. I don't know it it's worth it...

I think a part of the problem is that it's not fully clear that this _is_ a bug. I mean, at a first glance, being able to display the actual source code if we have it is good. But this "best effort" aspect of it means it's hard for a user to know what exactly is the debugger doing. I mean, the fact that a file has an older timestamp than the module does not actually mean the module was built from that source file. The behavior "I display the file on disk, and in case it is newer than the module, I print a warning" is pretty simple to understand. However, if you change that to "I display the file on disk, and in case it is newer than the module, I print a warning, *unless* I happen to have a cached version of that file which is older than the module, in which case I print that" gets a lot fuzzier and may get people to think that the debugger is actually smart enough to find the "real" source file, which is of course not true.

So, I'm not actually convinced that this is an improvement...

[n1tram1]

In D78421#2060327, @labath wrote:

So, I'm not actually convinced that this is an improvement...

This is exactly what is was worried about, I'm not sure either.

The only thing I thought was a bug is the fact the lldb shows you the modified file but the running module hasn't changed (GDB for example shows the previous file unless the executable is recompiled&reloaded).

But I'm not sure this is really a bug a worth fix fixing (I'm very worried about breaking other weird behavior), at least the current behavior is very understandable.

I think I'm going to close this issue.

lldb/include/lldb/Core/SourceManager.h

protected:
     uint32_t m_source_map_mod_id = 0;
     lldb::DataBufferSP m_data_sp;
     typedef std::vector<uint32_t> LineOffsets;
     LineOffsets m_offsets;
     lldb::DebuggerWP m_debugger_wp;
 
   private:
     void CommonInitializer(const FileSpec &file_spec, Target *target);
+
+    bool IsCurrentModuleAndNewer(llvm::sys::TimePoint<> time);
+    bool IsNewerThanItsModule(llvm::sys::TimePoint<> time);
+    void Update(llvm::sys::TimePoint<> mod_time);
   };
 
   typedef std::shared_ptr<File> 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.
   class SourceFileCache {

lldb/source/Core/SourceManager.cpp

 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/LineEntry.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Target/PathMappingList.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Target/Thread.h"
 #include "lldb/Utility/AnsiTerminal.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferLLVM.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-enumerations.h"
 
+#include "lldb/Utility/Log.h"
+
 #include "llvm/ADT/Twine.h"
 
 #include <memory>
 #include <utility>
 
 #include <assert.h>
 #include <stdio.h>
 
if (target) {
               }
             }
           }
           if (!got_multiple) {
             SymbolContext sc;
             sc_list.GetContextAtIndex(0, sc);
             if (sc.comp_unit)
               m_file_spec = sc.comp_unit->GetPrimaryFile();
-            m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
+            m_mod_time =
+                FileSystem::Instance().GetModificationTime(m_file_spec);
           }
         }
       }
       // Try remapping if m_file_spec does not correspond to an existing file.
       if (!FileSystem::Instance().Exists(m_file_spec)) {
         FileSpec new_file_spec;
         // Check target specific source remappings first, then fall back to
         // modules objects can have individual path remappings that were

 }
 
 void SourceManager::File::UpdateIfNeeded() {
   // 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);
 
+  // If the source file is newer than the module don't update,
+  // otherwise the source file being displayed will be different from
+  // the module being ran.
+  // (We only want to update if the module has been recompiled)
+  if (IsCurrentModuleAndNewer(curr_mod_time) ||
+      IsNewerThanItsModule(curr_mod_time))
+    // TODO: maybe issue a:
+    // 'warning: Source file is more recent than executable.' ?
+    return;
+
   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();
-  }
+    Update(curr_mod_time);
+  }
+}
+
+bool SourceManager::File::IsCurrentModuleAndNewer(llvm::sys::TimePoint<> time) {
+  DebuggerSP debugger_sp(m_debugger_wp.lock());
+  if (!debugger_sp)
+    return false;
+
+  lldb::TargetSP target_sp(debugger_sp->GetSelectedTarget());
+  if (!target_sp)
+    return false;
+
+  lldb::ProcessSP process_sp(target_sp->CalculateProcess());
+  if (!process_sp)
+    return false;
+
+  ThreadList &thread_list(process_sp->GetThreadList());
+
+  lldb::ThreadSP thread_sp(thread_list.GetSelectedThread());
+  if (!thread_sp)
+    return false;
+
+  lldb::StackFrameSP stackframe_sp(thread_sp->GetSelectedFrame());
+  if (!stackframe_sp)
+    return false;
+
+  const Address pc_addr(stackframe_sp->GetFrameCodeAddress());
+
+  lldb::ModuleSP current_module(pc_addr.GetModule());
+  if (!current_module)
+    return false;
+
+  SymbolContextList sc_list;
+  auto num_matches = current_module->ResolveSymbolContextsForFileSpec(
+      m_file_spec, 0, false, eSymbolContextModule | eSymbolContextCompUnit,
+      sc_list);
+
+  if (!num_matches)
+    return false;
+
+  // FIXME: We need to get the timestamp from the filesystem because the Module
+  // doesn't properly update its m_mod_time.
+  auto current_module_mod_time =
+      FileSystem::Instance().GetModificationTime(current_module->GetFileSpec());
+
+  return time > current_module_mod_time;
+}
+
+bool SourceManager::File::IsNewerThanItsModule(llvm::sys::TimePoint<> time) {
+  DebuggerSP debugger_sp(m_debugger_wp.lock());
+  if (!debugger_sp)
+    return false;
+
+  lldb::TargetSP target_sp(debugger_sp->GetSelectedTarget());
+  if (!target_sp)
+    return false;
+
+  SymbolContextList sc_list;
+  auto num_matches = target_sp->GetImages().ResolveSymbolContextsForFileSpec(
+      m_file_spec, 0, false, eSymbolContextModule | eSymbolContextCompUnit,
+      sc_list);
+
+  if (num_matches == 1) {
+    SymbolContext sc;
+    ModuleSP module_sp = nullptr;
+
+    sc_list.GetContextAtIndex(0, sc);
+    module_sp = sc.module_sp;
+
+    // FIXME: We need to get the timestamp from the filesystem because the
+    // Module doesn't properly update its m_mod_time.
+    auto module_mod_time =
+        FileSystem::Instance().GetModificationTime(module_sp->GetFileSpec());
+
+    if (module_sp)
+      return time > module_mod_time;
+  }
+
+  // TODO: if (num_matches > 1) warn multiple source files found matching
+
+  // We can't determine if the file is newer because we can't find its module.
+  return false;
+}
+
+void SourceManager::File::Update(llvm::sys::TimePoint<> modification_time) {
+  m_mod_time = modification_time;
 }
 
 size_t SourceManager::File::DisplaySourceLines(uint32_t line,
                                                llvm::Optional<size_t> column,
                                                uint32_t context_before,
                                                uint32_t context_after,
                                                Stream *s) {
   // Nothing to write if there's no stream.
bool SourceManager::File::GetLine(uint32_t line_no, std::string &buffer) {
   }
   buffer.assign((char *)m_data_sp->GetBytes() + start_offset,
                 end_offset - start_offset);
 
   return true;
 }
 
 void SourceManager::SourceFileCache::AddSourceFile(const FileSP &file_sp) {
-  FileSpec file_spec;
+  FileSpec file_spec(file_sp ? file_sp->GetFileSpec() : FileSpec());
   FileCache::iterator pos = m_file_cache.find(file_spec);
   if (pos == m_file_cache.end())
     m_file_cache[file_spec] = file_sp;
   else {
     if (file_sp != pos->second)
       m_file_cache[file_spec] = file_sp;
   }
 }

lldb/test/API/source-manager/TestSourceManager.py

def test_modify_source_file_while_debugging(self):
             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))
 
-        # Display the source code again.  We should see the updated line.
+        # Display the source code again.
+        # We should not see the updated line or there would be an incoherence
+        # with the module being ran.
         self.expect(
             "source list -f main-copy.c -l %d" %
             self.line,
             SOURCE_DISPLAYED_CORRECTLY,
-            substrs=['Hello lldb'])
+            substrs=['Hello world'])
 
     def test_set_breakpoint_with_absolute_path(self):
         self.build()
         hidden = self.getBuildArtifact("hidden")
         lldbutil.mkdir_p(hidden)
         # 'make -C' has resolved current directory to its realpath form.
         builddir_real = os.path.realpath(self.getBuildDir())
         hidden_real = os.path.realpath(hidden)