Page MenuHomePhabricator

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

Authored by alvinhochun on Sep 26 2022, 4:19 AM.

Details

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.

Diff Detail

Event Timeline

alvinhochun created this revision.Sep 26 2022, 4:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 4:19 AM
alvinhochun requested review of this revision.Sep 26 2022, 4:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 4:19 AM
mstorsjo added a subscriber: jasonmolenda.

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 accepted this revision.Sep 27 2022, 8:43 AM
labath added inline comments.
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.

This revision is now accepted and ready to land.Sep 27 2022, 8:43 AM

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.

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.

Updated the patch to be standalone.

alvinhochun edited the summary of this revision. (Show Details)

Actually update commit message

alvinhochun added inline comments.
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.

This revision was landed with ongoing or failed builds.Sep 30 2022, 3:53 AM
This revision was automatically updated to reflect the committed changes.