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

Thu Nov 11 13:09:37 PST 2021

jingham added a comment.

In D113521#3124239 <https://reviews.llvm.org/D113521#3124239>, @labath wrote:

> 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.)

This is the same situation as if you had attached to a PID on a remote with no local binary, then reran, right?  That should also work w/o the user having to intervene with a LaunchInfo for the second run, and so should re-running in this case.  We shouldn't NEED the extra launch info to make rerun in your case work.  And in general we don't ask users to respecify elements of the LaunchInfo like arguments and environments on rerun - we always reuse what you set the first time. That seems like a good principle to follow here as well.  So if this doesn't work, it's really a bug in our handling of the information we have after the first run exits, and doesn't seem like a very strong argument for case (b).

I don't think Greg's use case is instructive here either.  If you already have a binary locally, you can set its remote executable path directly, which is the extant way of specifying the remote path.  The other part of his ask was "give me a way not to install it" but the fact that you've set an ExecutableFile in your LaunchInfo doesn't seem like a good way to tell us that.  If we want to add that, then we should do it by adding a {Get/Set}ShouldInstallBinary to the LaunchInfo (*).

It seems to me the meaning of SetExecutableFile is clear if you don't have an executable module.  In all the other cases, we have to make decisions about what to do when this information conflicts with the information recorded in other places (currently the module's PlatformFileSpec and the Target's ProcessLaunchInfo).  We don't need to make those decisions to implement the case where it's clear what is meant however - i.e. the one where we have no executable.

I'm a little hesitant about making this an error without some way to make sure there isn't some other meaning of the LaunchInfo's ExecutableFile in the presence of an executable that I would break.  That seems hard to ensure, this code is pretty complicated as it stands.  It seems better not to make changes we don't have to.  But if you guys are confident, I don't mind adding that change.

(*) BTW, there's a separate issue here which I think Greg was also hinting at, which is that it was a bad choice to record the PlatformFile in the Module at all.  Modules are shared in lldb, and it's possible to have two debugging sessions using the same Module but targeting different remote devices with different install paths for the executable.  So having this info stored in the module is just asking for trouble, and is something we should remove and go to holding the remote path in the target only.  That would be a good cleanup, and I think it would unwind a lot of this complexity.  But that's a much bigger change, and not one I have time for at present.

>>> - 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;
----------------
labath wrote:
> 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.)
If you can't find the module, you aren't going to get any debug information, so this will be a pretty sad debugging session.  But I guess that's all the more reason not to make it sadder...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113521