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

Wed Nov 10 17:03:20 PST 2021

jingham added a comment.

In D113521#3123051 <https://reviews.llvm.org/D113521#3123051>, @clayborg 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).
>>
>> As you point out indirectly above, there are really three entities in play here.  There's the Target's ExecutableModule; there's the GetExecutableFile in a user-provided ProcessLaunchInfo passed to SBTarget::Launch, for instance; and there's the ProcessLaunchInfo that's stored in the TargetProperties held by the target.
>> So we need to decide what it means when these three entities differ.
>
> Shouldn't the target;s launch info get a full copy of the user-provided ProcessLaunchInfo upon launch? If that is the case, then we always can refer to the target's launch info as the truth as far as ProcessLaunchInfo goes? Then the only question we have is what to do if the target has a module
>
>> First let's address the externally supplied LaunchInfo.  Why would you make a Target with an executable module A, and then tell it to launch with a LaunchInfo whose GetExecutableFile is a different FileSpec from the one in the Executable Module? The only case where this seems useful is if there is no executable module.  That's bourne out by the implementation, where if the Target has an executable module the one in the LaunchInfo is ignored.  So maybe the right solution for this divergence is to make it an error to use a SBLaunchInfo with a different ExecutableFile.  Or maybe we could use the LaunchInfo's FileSpec as the remote file in the case where they've already given us a local file?  That's sort of coherent with the case where there was no local file.  In my patch I'm really using the GetExecutableFile to hold the remote path that we would have in the Module except that we don't have a way to make an Executable Module for the target that just has a remote file spec and nothing else.
>
> Maybe you don't have the platform implemented to install a binary and you have a local copy of the binary at "/local/a.out" and then you want the remote to launch "/remote/a.out" using this path from the launch info?
>
>> In the case of exec, I don't think we want to use values of the Executable Module stuffed into a user-provided LaunchInfo and then passed to Target::Launch or Process::Launch to make re-run work.  That would mean the user would have to keep around the ProcessLaunchInfo with the original binary and then feed it back to the target to get the launch to work, which seems awkward, and I don't see how you would do that well on the command line.
>
> It depends on what we currently do with the target's launch info. If it is still setup like it was before, then re-running should just work. If we allow the executable module in the target to take precedence, then we can't use it.
>
>> So you might think you should solve this by leaving the original launch information in the TargetProperties LaunchInfo, and then re-run would always use that.  But this has the problem that even though you KNOW on re-run you're first going to be debugging the initial binary, that won't be the main executable between runs, so setting breakpoints - if you want some to take in the initial binary - is going to be confusing.  To handle this situation gracefully, I think we'd need to reset the ExecutableModule in the target when the exec-ed binary exits, not just squirrel the binary FileSpec away in the TargetProperties ProcessLaunchInfo.
>
> Not sure I agree with that.
>
>> 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.
>
> true.

This seems like an interesting design discussion, but about issues that this patch doesn't address.  If we want to talk about this more, maybe do a thread on the dev list?  It's really easy to lose track of discussions from in patch reviews...

================
Comment at: lldb/source/Target/Process.cpp:2525
+  
+  if (exe_module && FileSystem::Instance().Exists(exe_module->GetFileSpec())) {
     // Install anything that might need to be installed prior to launching.
----------------
clayborg wrote:
> The other scenario I see to make it easy to have a local copy of the binary that we use for our target, and then still be able to just launch a binary for a different path on the remote device. Right now it seems that the only way to do this would be to create a target with no executable, and then specify the launch info's executable path. But it would be nice to be able to create a target with "~/a.out" without setting the platform path on the binary, and not having to install it via the platform, and have a "/remote/path/to/a.out" specified as the launch info's executable path to avoid having to copy it over. Is that currently possible?
There's two parts to what you want to do.  One is to bypass the Install in this scenario, and the other is to use the LaunchInfo at the point where we go to launch as the remote path if there's also an executable module.  

For the former, I think you would need the user to tell you not to install as a separate bit of info.  I don't think that the fact of the LaunchInfo having a different path directly implies you don't want to install it.  So we'd have to add some controls for that.

And then the second part is really what to do down in the guts of Launching when the LaunchInfo has a file and the Module has a file (and because there's no way say you can't do this currently, if the Module also has a remote file...)

That's part of the work that Pavel was hinting at, but not what this patch is doing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113521