[Lldb-commits] [PATCH] D113521: Allow lldb to launch a remote executable when there isn't a local copy

Thu Nov 11 04:56:55 PST 2021

labath added a comment.

In D113521#3122654 <https://reviews.llvm.org/D113521#3122654>, @jingham wrote:

> In D113521#3120703 <https://reviews.llvm.org/D113521#3120703>, @labath wrote:
>
>> I have two high-level questions about this:
>>
>> - what should be the relative priority of executable ModuleSP vs the launch info? IIUC, in the current implementation the module takes precedence, but it seems to me it would be more natural if the launch info came out on top. One of the reasons I'm thinking about this is because you currently cannot re-run executables that use exec(2) (I mean, you can, but it will rarely produce the desired results). This happens because we use the post-exec executable, but the rest of the launch info refers to the main one. If the executable module was not the primary source of truth here, then maybe the we could somehow use this mechanism to improve the exec story as well (by storing the original executable in the launch info or something).

> Anyway, I think you are asking the question "What should we do if the Target's ProcessLaunchInfo::GetExecutableFile differs from the one in the Target's Executable Module".  Or rather, should we keep the Target's ProcessLaunchInfo as the truth of what that target wants to launch, rather than looking at the Executable module.
>
> That question is orthogonal to the one this patch is determining, which is just about the case where there isn't an executable file in the target so that the user needs to provide this info from the outside.  So while I agree that yours is an interesting question, it isn't directly germane to this patch.

Yes, that is the question I am asking. I didn't want to go off into designing an exec feature. I only mentioned because it seemed potentially related. We can put away that for now.

While my question may not be "directly germane" to this patch, I wouldn't say it's orthogonal either. Right now (IIUC) the executable module is always the source of truth for the launch operation. Now you're adding a second source, so one has to choose how to handle the situation when both of them are present. You may not care what happens in that case, but _something_ is going to happen nonetheless. I guess we basically have three options:
a) prefer the executable module (this is what the patch implements, and it's also the smallest deviation from status quo of completely ignoring launch info)
b) prefer the launch info (that is what I proposed)
c) make it an error (I think that's something we could all agree on)

The reason I proposed (b) is because of the principle of specific overriding general. The executable module is embedded into the target, so I would consider it more general than the launch info, which can be provided directly to the Launch method. Greg's use case (launching a remote binary that you already have a copy of) seems like it could benefit from that. However, maybe I have an even better one. What would happen with reruns? On the first run, the user would create a executable-less target, and provide a binary through the launch info. The binary would launch fine and exit. Then the user would try to launch again using the same target and launch info, but the code would go down a different path (I'm not sure which one) because the target would already have an executable. (This is actually kind of similar to the exec scenario, because the executable module would change between the two runs.)

>> - what happens if the launch_info path happens to refer to a host executable? Will we still launch the remote one or will we try to copy the local one instead? I'm not sure of the current behavior, but I would like to avoid the behavior depending on the presence of local files.
>
> Everything in Process::Launch, which is what does the real work for this part of the Launch, uses the return from GetTarget().GetExecutableModulePointer() for its "host side" work.  That's always going to be null in the scenario I'm adding support for.  So we won't look at the local file system at all till the launch succeeds and the process stops and we start querying the dynamic loader for executables, and go looking for local copies like we always do.

Ok, that sounds great.

================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:268-269
+          exe_module_sp = target->GetExecutableModule();
+        if (!exe_module_sp) {
+          result.AppendError("Could not get executable module after launch.");
+          return false;
----------------
jingham wrote:
> clayborg wrote:
> > I agree with Pavel here, unless you have a case where you need this to work this way?
> If we couldn't get the executable module, that would mean that we queried a process stopped in the remote server after launch, and the dynamic loader wasn't able to determine the main executable.  That's something we do all the time, so it would be weird if we couldn't do that.  I don't know why that would happen or how well the debug session is going to go on from here.  But leaving it around in the hopes that it's useful is fine by me.
I believe folks doing bare-metal debugging often have executable-less processes. It's probably not the most tested path, but it's one I believe we should (ought to) support. Not finding a module after a launch is fairly unexpected though, so a warning is appropriate. (It's easier for this to happen with elf targets, since one cannot really read an elf file from memory there (memory image does not contain all of it), though we would be able to read it from disk most of the time, I believe.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113521