[lldb][Windows] Always call SetExecutableModule on debugger connected

[alvinhochun]

Summary

In ProcessWindows::OnDebuggerConnected (triggered from
CREATE_PROCESS_DEBUG_EVENT), we should always call
Target::SetExecutableModule regardless of whether LLDB has already
preloaded the executable modules. SetExecutableModule has the side
effect of clearing the module list of the Target, which help make sure
that module #0 is the executable module and the rest of the modules are
listed according to the DLL load order in the process (technically this
has no real consequences but it seems to make more sense anyway.) It
also fixes an issue where the modules preloaded by LLDB will be
duplicated when the debuggee process actually loads the DLL.

[mstorsjo]

The test runs fine on Windows, but I'm not familiar with these aspects of lldb to (yet) have a good opinion on it; adding @jasonmolenda to the review who commented on the previous one too.

[labath]

lldb/test/Shell/Target/dependent-modules-nodupe-windows.test

21–22

I'm not sure if hardcoding the order of system libraries (something which you have no control of) is such a good idea.

[alvinhochun]

Okay, I'll rebase the change to remove the dependency on D134581.

lldb/test/Shell/Target/dependent-modules-nodupe-windows.test

21–22

I think it's fine-ish. AFAIK ntdll.dll is pretty much guaranteed to be loaded by the loader ahead of other DLLs and kernel32.dll is depended on by almost everything else. It is definitely an implementation detail though so I can understand the concern.

[jasonmolenda]

This looks good to me, fwiw. I agree with Pavel about hardcoding the order that libraries might appear unless that ordering is essentially considered API. We could get in a situation where different versions of windows report the binaries in different order, and it might be confusing for people to understand why a bot is failing when their desktop passes etc. Not a platform I know anything about though, don't have a real objection.

[alvinhochun]

Updated the patch to be standalone.

[alvinhochun]

Actually update commit message

[alvinhochun]

lldb/test/Shell/Target/dependent-modules-nodupe-windows.test

7–10

For reference, these test commands were discussed in https://reviews.llvm.org/D134581#inline-1297440.

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

 }
 
 void ProcessWindows::OnDebuggerConnected(lldb::addr_t image_base) {
   DebuggerThreadSP debugger = m_session_data->m_debugger;
   Log *log = GetLog(WindowsLog::Process);
   LLDB_LOG(log, "Debugger connected to process {0}.  Image base = {1:x}",
            debugger->GetProcess().GetProcessId(), image_base);
 
-  ModuleSP module = GetTarget().GetExecutableModule();
-  if (!module) {
-    // During attach, we won't have the executable module, so find it now.
-    const DWORD pid = debugger->GetProcess().GetProcessId();
-    const std::string file_name = GetProcessExecutableName(pid);
-    if (file_name.empty()) {
-      return;
-    }
+  ModuleSP module;
 
-    FileSpec executable_file(file_name);
-    FileSystem::Instance().Resolve(executable_file);
-    ModuleSpec module_spec(executable_file);
-    Status error;
-    module =
-        GetTarget().GetOrCreateModule(module_spec, true /* notify */, &error);
-    if (!module) {
-      return;
-    }
 
-    GetTarget().SetExecutableModule(module, eLoadDependentsNo);
-  }
 
   if (auto dyld = GetDynamicLoader())
     dyld->OnLoadModule(module, ModuleSpec(), image_base);
 
   // Add the main executable module to the list of pending module loads.  We
   // can't call GetTarget().ModulesDidLoad() here because we still haven't
   // returned from DoLaunch() / DoAttach() yet so the target may not have set
   // the process instance to `this` yet.

lldb/test/Shell/Target/Inputs/main.c

+__declspec(dllimport) void exportFunc(void);
+int main() { exportFunc(); }

lldb/test/Shell/Target/Inputs/shlib.c

+__declspec(dllexport) void exportFunc(void) {}

lldb/test/Shell/Target/dependent-modules-nodupe-windows.test

+# 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:             %if windows-msvc %{-Wl,-implib:%t.shlib.lib%} \
+# RUN:             %else %{-Wl,--out-implib=%t.shlib.lib%}
+# RUN: %clang_host -g0 -O0 %S/Inputs/main.c %t.shlib.lib -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: ntdll.dll
+# CHECK-NEXT: kernel32.dll
+# CHECK:      .shlib.dll
+# CHECK-NOT:  .shlib.dll