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

557

if (!debugger_sp)
  return false;

559

if (!target_sp)
  return false;

560

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

576

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:
     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 IsNewerThanDebuggedExecutable(llvm::sys::TimePoint<> time);
+    bool IsNewerThanCurrentModule(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.

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"
 

 }
 
 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 (IsNewerThanDebuggedExecutable(curr_mod_time))
+  // If the source file is newer than the executable don't update,
+  // otherwise the source file being displayed will be different from
+  // the executable being ran.
+  // (We only want to update if the executable has been recompiled)
+  if (IsNewerThanCurrentModule(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) {
     Update(curr_mod_time);
   }
 }
 
-bool SourceManager::File::IsNewerThanDebuggedExecutable(
+bool SourceManager::File::IsNewerThanCurrentModule(
     llvm::sys::TimePoint<> time) {
-  // Check if we are running in a debugger.
   DebuggerSP debugger_sp(m_debugger_wp.lock());
-  if (debugger_sp) {
-    lldb::TargetSP target_sp(debugger_sp->GetSelectedTarget());
-    if (target_sp) {
-      lldb::ModuleSP exec_module_sp(target_sp->GetExecutableModule());
-      if (exec_module_sp) {
-        auto exec_module_mod_time = exec_module_sp->GetModificationTime();
+  if (!debugger_sp)
+    return false;
+
+  if (!target_sp)
+    return false;
 
-        // If the source file is newer than the executable don't update,
-        // otherwise the source file being displayed will be different from
-        // the executable being ran.
-        // (We only want to update if the executable has been recompiled)
-        if (time > exec_module_mod_time) {
-          // TODO: maybe issue a:
-          //       'warning: Source file is more recent than executable.' ?
-          return true;
-        }
-      }
-    }
-  }
+  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;
+
+  auto current_module_mod_time = current_module->GetModificationTime();
+
+  if (time <= current_module_mod_time) {
+    return false;
+  }
+
+  return true;
 }
 
 void SourceManager::File::Update(llvm::sys::TimePoint<> modification_time) {
   m_data_sp = FileSystem::Instance().CreateDataBuffer(m_file_spec);
   m_offsets.clear();
   m_mod_time = modification_time;
 }