[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a reviewer: DavidSpickett.
alvinhochun added a comment.

Thanks for the comment.

In D134581#3813484 , @jasonmolenda 
wrote:

> I probably misunderstand the situation DynamicLoaderWindows finds itself in, 
> but in DynamicLoaderDarwin we have similar behavior - you run 'lldb binary' 
> and lldb loads all the directly linked dynamic libraries into the target it 
> creates, pre-execution.  When execution starts, the dynamic linker tells us 
> where the binary is loaded in memory and we call Target::SetExecutableModule 
> with it.  Target::SetExecutableModule has a side effect of clearing the 
> Target's module list - you now have only one binary in the Target, your 
> executable module.  (this is done so the 0th image in the image list is your 
> executable module)
>
> Why aren't your pre-loaded DLLs unloaded when you call 
> Target::SetExecutableModule?

In `ProcessWindows::OnDebuggerConnected`, it first checks 
`GetTarget().GetExecutableModule()`. Only when the returned module is null 
(i.e. the binary hasn't already been loaded) then it calls 
`GetTarget().SetExecutableModule(module, eLoadDependentsNo)`. If I understood 
you correctly, then the right thing to do there should be to call 
`GetTarget().SetExecutableModule(module, eLoadDependentsNo)` in all cases, 
without checking `GetExecutableModule`, right?

It seems to make sense, but I may need some comments from other reviewers on 
this.

In D134581#3813485 , @jasonmolenda 
wrote:

> I'll have to admit, a quick read through of Target::GetOrAddModule() worries 
> me about adding this mid-function return - the code clearly has a mix of 
> cases where it has found a Module on disk that is new to lldb and is loading 
> it, it has found a module in lldb's global module cache which has the same 
> UUID and/or filepath, and something in there about a Target which already has 
> a module and the newly loaded module is used to replace it - not sure what 
> that's about, but I only read it quickly.  My initial impression is that this 
> change is unlikely to be the right thing to do, tbh, regardless of the more 
> basic "why are we in this situation in the first place".   Again, I might be 
> mistaken/not understanding the issue properly though.

I checked again and I think you are probably correct. Other than 
`!did_create_module` it seems an additional condition `&& 
m_images.FindModule(module_sp.get())` will be needed for it to do the right 
thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134581/new/

https://reviews.llvm.org/D134581

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun updated this revision to Diff 462745.
alvinhochun added a comment.

Updated condition for early return


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134581/new/

https://reviews.llvm.org/D134581

Files:
  lldb/source/Target/Target.cpp
  lldb/test/Shell/Target/Inputs/main.c
  lldb/test/Shell/Target/Inputs/shlib.c
  lldb/test/Shell/Target/dependent-modules-nodupe-windows.test


Index: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
===
--- /dev/null
+++ lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
@@ -0,0 +1,20 @@
+# REQUIRES: system-windows
+
+# Checks that dependent modules preloaded by LLDB are not duplicated when the
+# process actually loads the DLL.
+
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \
+# RUN:   -o "#after" -o "target modules list" %t.main.exe | FileCheck %s
+
+# CHECK-LABEL: #before
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: .shlib.dll
+
+# CHECK-LABEL: #after
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: .shlib.dll
+# CHECK-NOT:  .shlib.dll
Index: lldb/test/Shell/Target/Inputs/shlib.c
===
--- /dev/null
+++ lldb/test/Shell/Target/Inputs/shlib.c
@@ -0,0 +1 @@
+__declspec(dllexport) void exportFunc(void) {}
Index: lldb/test/Shell/Target/Inputs/main.c
===
--- /dev/null
+++ lldb/test/Shell/Target/Inputs/main.c
@@ -0,0 +1,2 @@
+__declspec(dllimport) void exportFunc(void);
+int main() { exportFunc(); }
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2134,6 +2134,14 @@
   }
 }
 
+// If we found a pre-existing module, just return it.
+if (module_sp && !did_create_module &&
+m_images.FindModule(module_sp.get())) {
+  if (error_ptr)
+*error_ptr = error;
+  return module_sp;
+}
+
 // We found a module that wasn't in our target list.  Let's make sure that
 // there wasn't an equivalent module in the list already, and if there was,
 // let's remove it.


Index: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
===
--- /dev/null
+++ lldb/test/Shell/Target/dependent-modules-nodupe-windows.test
@@ -0,0 +1,20 @@
+# REQUIRES: system-windows
+
+# Checks that dependent modules preloaded by LLDB are not duplicated when the
+# process actually loads the DLL.
+
+# RUN: %clang_host -g0 -O0 -shared %S/Inputs/shlib.c -o %t.shlib.dll
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.dll -o %t.main.exe
+# RUN: %lldb -b -o "#before" -o "target modules list" -o "b main" -o run \
+# RUN:   -o "#after" -o "target modules list" %t.main.exe | FileCheck %s
+
+# CHECK-LABEL: #before
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: .shlib.dll
+
+# CHECK-LABEL: #after
+# CHECK-NEXT: target modules list
+# CHECK-NEXT: .main.exe
+# CHECK-NEXT: .shlib.dll
+# CHECK-NOT:  .shlib.dll
Index: lldb/test/Shell/Target/Inputs/shlib.c
===
--- /dev/null
+++ lldb/test/Shell/Target/Inputs/shlib.c
@@ -0,0 +1 @@
+__declspec(dllexport) void exportFunc(void) {}
Index: lldb/test/Shell/Target/Inputs/main.c
===
--- /dev/null
+++ lldb/test/Shell/Target/Inputs/main.c
@@ -0,0 +1,2 @@
+__declspec(dllimport) void exportFunc(void);
+int main() { exportFunc(); }
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2134,6 +2134,14 @@
   }
 }
 
+// If we found a pre-existing module, just return it.
+if (module_sp && !did_create_module &&
+m_images.FindModule(module_sp.get())) {
+  if (error_ptr)
+*error_ptr = error;
+  return module_sp;
+}
+
 // We found a module that wasn't in our target list.  Let's make sure that
 // there wasn't an equivalent module in the list already, and if there was,
 // let's remove it.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits