Allow lldb to launch a remote executable when there isn't a local copy

[jingham]

Summary

lldb doesn't need to have a local copy of the binary that it is going to ask the remote stub to run for it, it can send the remote stub a path to the binary, and then read the executable module out of memory once the binary has been launched.

There is a workflow for providing an alternate remote path, but it hangs the information off the executable module, which if you don't have a local copy of the file you can't make.

So I came up with an alternate workflow for the case where you don't have a local executable copy, using LaunchInfo::SetExecutableFile. If you tell a target or connected process to Launch, and the LaunchInfo has an executable file but the target doesn't, then send the LaunchInfo's executable file to the remote stub, to see if it can launch it.

[labath]

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

lldb/source/Commands/CommandObjectProcess.cpp

269

This probably shouldn't be a hard error. If we fail to obtain the executable module, I'd think we should still be able to continue with asm debugging. (In fact, since you don't clean up the process here, that's probably what will happen regardless of the error state.)

lldb/test/API/functionalities/gdb_remote_client/TestNoLocalFile.py

31

the responder will run on a separate thread, so a failing assertion will not immediately terminate the test, but rather confuse the hell out of everyone. It be better to just record the data here, and do the assertion on the main thread.

70

I would use the CreateTarget overload which takes a platform name argument. Otherwise, this will pick a random platform on non-darwin hosts and then it may get confused by the darwin-specific responses.

80–84

we have lldbutil.expect_state_changes for this

[jingham]

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

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.

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.

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.

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.

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

I'll reply to the individual comments after lunch...

[clayborg]

In D113521#3122654, @jingham wrote:

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

lldb/source/Commands/CommandObjectProcess.cpp

268–269

I agree with Pavel here, unless you have a case where you need this to work this way?

lldb/source/Target/Process.cpp

2496–2497

maybe switch these over to std::string values and then use the FileSpec::GetPath() that returns a std::string below? It would be nice to remove any PATH_MAX stuff from our generic code if possible.

2514–2515

If we switch to std:string for platform_exec_file_path this becomes:

platform_exec_file_path = exe_spec_to_use.GetPath();

2518–2519

If we switch to std:string for local_exec_file_path this becomes:

local_exec_file_path = exe_spec_to_use.GetPath();

2524

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?

[jingham]

Address Pavel's review comments

[jingham]

lldb/source/Commands/CommandObjectProcess.cpp

268–269

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.

lldb/test/API/functionalities/gdb_remote_client/TestNoLocalFile.py

31

Got it. I just preserve the file name from the A packet and test it later.

70

Good catch.

80–84

I hadn't noticed that, nice...

[jingham]

In D113521#3123051, @clayborg wrote:

In D113521#3122654, @jingham wrote:

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

lldb/source/Target/Process.cpp

2524

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.

[jingham]

Address Greg's review comments.

[jingham]

lldb/source/Target/Process.cpp

2496–2497

Turns out that the remote path string wasn't used at all, and the local one was only used for error reporting. So I deleted the one, and moved the other to where we report the error (and used a std::string)...

[labath]

In D113521#3122654, @jingham wrote:

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

lldb/source/Commands/CommandObjectProcess.cpp

268–269

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

[clayborg]

I'm good with this to get things started. Pavel?

[jingham]

In D113521#3124239, @labath wrote:

In D113521#3122654, @jingham wrote:

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

lldb/source/Commands/CommandObjectProcess.cpp

268–269

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

[labath]

In D113521#3125429, @clayborg wrote:

I'm good with this to get things started. Pavel?

I think I mostly understand where's Jim's coming from, though I don't fully agree with the conclusions. But, it's not my feature, so...

In D113521#3125597, @jingham wrote:

In D113521#3124239, @labath wrote:

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?

I don't have an intuition for what should happen in that case, but I suppose it could work that way.

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.

Yes, they don't NEED to provide it, but they CAN do so. If they provide the exact same information, then the situation would be as if they didn't provide anything. And I think that's your point here -- if the user provides the exact same path in the launch info, then he won't be able to tell the difference.
*However*, the user can also specify different launch info on the second launch. If he passes say, different environment (e.g., through SBTarget::Launch(SBLaunchInfo)), then these will take precedence over the ones stored in the target, right? Same for arguments, working dir, etc. But if the user specifies a different executable file, then it will be ignored.

I think I now understand how you look at this, but it this discrepancy still seems unfortunate...

lldb/source/Commands/CommandObjectProcess.cpp

 
 protected:
   bool DoExecute(Args &launch_args, CommandReturnObject &result) override {
     Debugger &debugger = GetDebugger();
     Target *target = debugger.GetSelectedTarget().get();
     // If our listener is nullptr, users aren't allows to launch
     ModuleSP exe_module_sp = target->GetExecutableModule();
 
-    if (exe_module_sp == nullptr) {
+    // If the target already has an executable module, then use that.  If it
+    // doesn't then someone must be trying to launch using a path that will
+    // make sense to the remote stub, but doesn't exist on the local host.
+    // In that case use the ExecutableFile that was set in the target's
+    // ProcessLaunchInfo.
+    if (exe_module_sp == nullptr && !target->GetProcessLaunchInfo().GetExecutableFile()) {
       result.AppendError("no file in target, create a debug target using the "
                          "'target create' command");
       return false;
     }
 
     StateType state = eStateInvalid;
 
     if (!StopProcessIfNecessary(m_exe_ctx.GetProcessPtr(), state, result))
bool DoExecute(Args &launch_args, CommandReturnObject &result) override {
     m_options.launch_info.GetEnvironment().insert(target_env.begin(),
                                                   target_env.end());
 
     llvm::StringRef target_settings_argv0 = target->GetArg0();
 
     if (!target_settings_argv0.empty()) {
       m_options.launch_info.GetArguments().AppendArgument(
           target_settings_argv0);
+      if (exe_module_sp)
         m_options.launch_info.SetExecutableFile(
             exe_module_sp->GetPlatformFileSpec(), false);
+      else
+        m_options.launch_info.SetExecutableFile(target->GetProcessLaunchInfo().GetExecutableFile(), false);
     } else {
+      if (exe_module_sp)
         m_options.launch_info.SetExecutableFile(
             exe_module_sp->GetPlatformFileSpec(), true);
+      else
+        m_options.launch_info.SetExecutableFile(target->GetProcessLaunchInfo().GetExecutableFile(), true);
     }
 
     if (launch_args.GetArgumentCount() == 0) {
       m_options.launch_info.GetArguments().AppendArguments(
           target->GetProcessLaunchInfo().GetArguments());
     } else {
       m_options.launch_info.GetArguments().AppendArguments(launch_args);
       // Save the arguments for subsequent runs in the current target.
if (error.Success()) {
         // stack to the main command handler and show an (lldb) prompt before
         // HandlePrivateEvent (from PrivateStateThread) has a chance to call
         // PushProcessIOHandler().
         process_sp->SyncIOHandler(0, std::chrono::seconds(2));
 
         llvm::StringRef data = stream.GetString();
         if (!data.empty())
           result.AppendMessage(data);
+        // If we didn't have a local executable, then we wouldn't have had an
+        // executable module before launch.
+        if (!exe_module_sp)
+          exe_module_sp = target->GetExecutableModule();
+        if (!exe_module_sp) {
+          result.AppendWarning("Could not get executable module after launch.");
+        } else {
+
           const char *archname =
               exe_module_sp->GetArchitecture().GetArchitectureName();
           result.AppendMessageWithFormat(
               "Process %" PRIu64 " launched: '%s' (%s)\n", process_sp->GetID(),
               exe_module_sp->GetFileSpec().GetPath().c_str(), archname);
+        }
         result.SetStatus(eReturnStatusSuccessFinishResult);
         result.SetDidChangeProcessState(true);
       } else {
         result.AppendError(
             "no error returned from Target::Launch, and target has no process");
       }
     } else {
       result.AppendError(error.AsCString());

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Status ProcessGDBRemote::DoLaunch(lldb_private::Module *exe_module,
   }
 
   //  ::LogSetBitMask (GDBR_LOG_DEFAULT);
   //  ::LogSetOptions (LLDB_LOG_OPTION_THREADSAFE |
   //  LLDB_LOG_OPTION_PREPEND_TIMESTAMP |
   //  LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD);
   //  ::LogSetLogFile ("/dev/stdout");
 
-  ObjectFile *object_file = exe_module->GetObjectFile();
-  if (object_file) {
-    error = EstablishConnectionIfNeeded(launch_info);
-    if (error.Success()) {
-      PseudoTerminal pty;
-      const bool disable_stdio = (launch_flags & eLaunchFlagDisableSTDIO) != 0;
 
-      PlatformSP platform_sp(GetTarget().GetPlatform());
-      if (disable_stdio) {
-        // set to /dev/null unless redirected to a file above
-        if (!stdin_file_spec)
-          stdin_file_spec.SetFile(FileSystem::DEV_NULL,
-                                  FileSpec::Style::native);
-        if (!stdout_file_spec)
-          stdout_file_spec.SetFile(FileSystem::DEV_NULL,
-                                   FileSpec::Style::native);
-        if (!stderr_file_spec)
-          stderr_file_spec.SetFile(FileSystem::DEV_NULL,
-                                   FileSpec::Style::native);
-      } else if (platform_sp && platform_sp->IsHost()) {
-        // If the debugserver is local and we aren't disabling STDIO, lets use
-        // a pseudo terminal to instead of relying on the 'O' packets for stdio
-        // since 'O' packets can really slow down debugging if the inferior
-        // does a lot of output.
-        if ((!stdin_file_spec || !stdout_file_spec || !stderr_file_spec) &&
-            !errorToBool(pty.OpenFirstAvailablePrimary(O_RDWR | O_NOCTTY))) {
-          FileSpec secondary_name(pty.GetSecondaryName());
 
-          if (!stdin_file_spec)
-            stdin_file_spec = secondary_name;
 
-          if (!stdout_file_spec)
-            stdout_file_spec = secondary_name;
 
-          if (!stderr_file_spec)
-            stderr_file_spec = secondary_name;
-        }
-        LLDB_LOGF(
-            log,
-            "ProcessGDBRemote::%s adjusted STDIO paths for local platform "
-            "(IsHost() is true) using secondary: stdin=%s, stdout=%s, "
-            "stderr=%s",
-            __FUNCTION__,
-            stdin_file_spec ? stdin_file_spec.GetCString() : "<null>",
-            stdout_file_spec ? stdout_file_spec.GetCString() : "<null>",
-            stderr_file_spec ? stderr_file_spec.GetCString() : "<null>");
-      }
 
-      LLDB_LOGF(log,
-                "ProcessGDBRemote::%s final STDIO paths after all "
-                "adjustments: stdin=%s, stdout=%s, stderr=%s",
-                __FUNCTION__,
-                stdin_file_spec ? stdin_file_spec.GetCString() : "<null>",
-                stdout_file_spec ? stdout_file_spec.GetCString() : "<null>",
-                stderr_file_spec ? stderr_file_spec.GetCString() : "<null>");
 
-      if (stdin_file_spec)
-        m_gdb_comm.SetSTDIN(stdin_file_spec);
-      if (stdout_file_spec)
-        m_gdb_comm.SetSTDOUT(stdout_file_spec);
-      if (stderr_file_spec)
-        m_gdb_comm.SetSTDERR(stderr_file_spec);
 
-      m_gdb_comm.SetDisableASLR(launch_flags & eLaunchFlagDisableASLR);
-      m_gdb_comm.SetDetachOnError(launch_flags & eLaunchFlagDetachOnError);
 
-      m_gdb_comm.SendLaunchArchPacket(
-          GetTarget().GetArchitecture().GetArchitectureName());
 
-      const char *launch_event_data = launch_info.GetLaunchEventData();
-      if (launch_event_data != nullptr && *launch_event_data != '\0')
-        m_gdb_comm.SendLaunchEventDataPacket(launch_event_data);
 
-      if (working_dir) {
-        m_gdb_comm.SetWorkingDir(working_dir);
-      }
 
-      // Send the environment and the program + arguments after we connect
-      m_gdb_comm.SendEnvironment(launch_info.GetEnvironment());
 
-      {
-        // Scope for the scoped timeout object
-        GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm,
-                                                      std::chrono::seconds(10));
 
-        int arg_packet_err = m_gdb_comm.SendArgumentsPacket(launch_info);
-        if (arg_packet_err == 0) {
-          std::string error_str;
-          if (m_gdb_comm.GetLaunchSuccess(error_str)) {
-            SetID(m_gdb_comm.GetCurrentProcessID());
-          } else {
-            error.SetErrorString(error_str.c_str());
-          }
-        } else {
-          error.SetErrorStringWithFormat("'A' packet returned an error: %i",
-                                         arg_packet_err);
-        }
-      }
 
-      if (GetID() == LLDB_INVALID_PROCESS_ID) {
-        LLDB_LOGF(log, "failed to connect to debugserver: %s",
-                  error.AsCString());
-        KillDebugserverProcess();
-        return error;
-      }
 
-      StringExtractorGDBRemote response;
-      if (m_gdb_comm.GetStopReply(response)) {
-        SetLastStopPacket(response);
 
-        const ArchSpec &process_arch = m_gdb_comm.GetProcessArchitecture();
 
-        if (process_arch.IsValid()) {
-          GetTarget().MergeArchitecture(process_arch);
-        } else {
-          const ArchSpec &host_arch = m_gdb_comm.GetHostArchitecture();
-          if (host_arch.IsValid())
-            GetTarget().MergeArchitecture(host_arch);
-        }
 
-        SetPrivateState(SetThreadStopInfo(response));
 
-        if (!disable_stdio) {
-          if (pty.GetPrimaryFileDescriptor() != PseudoTerminal::invalid_fd)
-            SetSTDIOFileDescriptor(pty.ReleasePrimaryFileDescriptor());
-        }
-      }
-    } else {
-      LLDB_LOGF(log, "failed to connect to debugserver: %s", error.AsCString());
-    }
-  } else {
-    // Set our user ID to an invalid process ID.
-    SetID(LLDB_INVALID_PROCESS_ID);
-    error.SetErrorStringWithFormat(
-        "failed to get object file from '%s' for arch %s",
-        exe_module->GetFileSpec().GetFilename().AsCString(),
-        exe_module->GetArchitecture().GetArchitectureName());
-  }
   return error;
 }
 
 Status ProcessGDBRemote::ConnectToDebugserver(llvm::StringRef connect_url) {
   Status error;
   // Only connect if we have a valid connect URL
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
 

lldb/source/Target/Process.cpp

Status Process::Launch(ProcessLaunchInfo &launch_info) {
   m_abi_sp.reset();
   m_dyld_up.reset();
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
   m_process_input_reader.reset();
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
+
+  // The "remote executable path" is hooked up to the local Executable
+  // module.  But we should be able to debug a remote process even if the
+  // executable module only exists on the remote.  However, there needs to
+  // be a way to express this path, without actually having a module.
+  // The way to do that is to set the ExecutableFile in the LaunchInfo.
+  // Figure that out here:
+  
+  FileSpec exe_spec_to_use;
   if (!exe_module) {
+    if (!launch_info.GetExecutableFile()) {
       error.SetErrorString("executable module does not exist");
       return error;
     }
+    exe_spec_to_use = launch_info.GetExecutableFile();
+  } else
+    exe_spec_to_use = exe_module->GetFileSpec();
   
-  char local_exec_file_path[PATH_MAX];
-  char platform_exec_file_path[PATH_MAX];
-  exe_module->GetFileSpec().GetPath(local_exec_file_path,
-                                    sizeof(local_exec_file_path));
-  exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
-                                            sizeof(platform_exec_file_path));
-  if (FileSystem::Instance().Exists(exe_module->GetFileSpec())) {
+  if (exe_module && FileSystem::Instance().Exists(exe_module->GetFileSpec())) {
     // Install anything that might need to be installed prior to launching.
     // For host systems, this will do nothing, but if we are connected to a
     // remote platform it will install any needed binaries
     error = GetTarget().Install(&launch_info);
     if (error.Fail())
       return error;
-
-    // Listen and queue events that are broadcasted during the process launch.
-    ListenerSP listener_sp(Listener::MakeListener("LaunchEventHijack"));
-    HijackProcessEvents(listener_sp);
-    auto on_exit = llvm::make_scope_exit([this]() { RestoreProcessEvents(); });
+  }
 
-    if (PrivateStateThreadIsValid())
-      PausePrivateStateThread();
 
-    error = WillLaunch(exe_module);
-    if (error.Success()) {
-      const bool restarted = false;
-      SetPublicState(eStateLaunching, restarted);
-      m_should_detach = false;
 
-      if (m_public_run_lock.TrySetRunning()) {
-        // Now launch using these arguments.
-        error = DoLaunch(exe_module, launch_info);
-      } else {
-        // This shouldn't happen
-        error.SetErrorString("failed to acquire process run lock");
-      }
 
-      if (error.Fail()) {
-        if (GetID() != LLDB_INVALID_PROCESS_ID) {
-          SetID(LLDB_INVALID_PROCESS_ID);
-          const char *error_string = error.AsCString();
-          if (error_string == nullptr)
-            error_string = "launch failed";
-          SetExitStatus(-1, error_string);
-        }
-      } else {
-        EventSP event_sp;
 
-        // Now wait for the process to launch and return control to us, and then
-        // call DidLaunch:
-        StateType state = WaitForProcessStopPrivate(event_sp, seconds(10));
 
-        if (state == eStateInvalid || !event_sp) {
-          // We were able to launch the process, but we failed to catch the
-          // initial stop.
-          error.SetErrorString("failed to catch stop after launch");
-          SetExitStatus(0, "failed to catch stop after launch");
-          Destroy(false);
-        } else if (state == eStateStopped || state == eStateCrashed) {
-          DidLaunch();
 
-          DynamicLoader *dyld = GetDynamicLoader();
-          if (dyld)
-            dyld->DidLaunch();
 
-          GetJITLoaders().DidLaunch();
 
-          SystemRuntime *system_runtime = GetSystemRuntime();
-          if (system_runtime)
-            system_runtime->DidLaunch();
 
-          if (!m_os_up)
-            LoadOperatingSystemPlugin(false);
 
-          // We successfully launched the process and stopped, now it the
-          // right time to set up signal filters before resuming.
-          UpdateAutomaticSignalFiltering();
 
-          // Note, the stop event was consumed above, but not handled. This
-          // was done to give DidLaunch a chance to run. The target is either
-          // stopped or crashed. Directly set the state.  This is done to
-          // prevent a stop message with a bunch of spurious output on thread
-          // status, as well as not pop a ProcessIOHandler.
-          // We are done with the launch hijack listener, and this stop should
-          // go to the public state listener:
-          RestoreProcessEvents();
-          SetPublicState(state, false);
 
-          if (PrivateStateThreadIsValid())
-            ResumePrivateStateThread();
-          else
-            StartPrivateStateThread();
 
-          // Target was stopped at entry as was intended. Need to notify the
-          // listeners about it.
-          if (state == eStateStopped &&
-              launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
-            HandlePrivateEvent(event_sp);
-        } else if (state == eStateExited) {
-          // We exited while trying to launch somehow.  Don't call DidLaunch
-          // as that's not likely to work, and return an invalid pid.
-          HandlePrivateEvent(event_sp);
-        }
-      }
-    }
   } else {
+    std::string local_exec_file_path = exe_spec_to_use.GetPath();
     error.SetErrorStringWithFormat("file doesn't exist: '%s'",
-                                   local_exec_file_path);
+                                   local_exec_file_path.c_str());
   }
 
   return error;
 }
 
 Status Process::LoadCore() {
   Status error = DoLoadCore();
   if (error.Success()) {

lldb/test/API/functionalities/gdb_remote_client/TestNoLocalFile.py

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestNoLocalFile(GDBRemoteTestBase):
+    """ Test the case where there is NO local copy of the file
+        being debugged.  We shouldn't immediately error out, but
+        rather lldb should ask debugserver if it knows about the file. """
+
+    
+    @skipIfXmlSupportMissing
+    def test(self):
+        self.absent_file = '/nosuch_dir/nosuch_subdir/nosuch_executable'
+        self.a_packet_file = None
+        class MyResponder(MockGDBServerResponder):
+            def __init__(self, testcase):
+                MockGDBServerResponder.__init__(self)
+                self.after_launch = False
+                self.testcase = testcase
+                self.current_thread = 0
+                
+            def A(self, packet):
+                # This is the main test, we want to see that lldb DID send the
+                # A packet to get debugserver to load the file.
+                # Skip the length and second length:
+                a_arr = packet.split(",")
+                self.testcase.a_packet_file = bytearray.fromhex(a_arr[2]).decode()
+                return "OK"
+
+            def qXferRead(self, obj, annex, offset, length):
+                if annex == "target.xml":
+                    return """<?xml version="1.0"?>
+                        <target version="1.0">
+                          <architecture>i386:x86-64</architecture>
+                          <feature name="org.gnu.gdb.i386.core">
+                            <reg name="rip" bitsize="64" regnum="0" type="code_ptr" group="general"/>
+                          </feature>
+                        </target>""", False
+                else:
+                    return None, False
+
+            def qC(self):
+                if not self.after_launch:
+                    return "QC0"
+                return "0"
+
+            def qfThreadInfo(self):
+                if not self.after_launch:
+                    return "OK"
+                return "m0"
+
+            def qsThreadInfo(self):
+                if not self.after_launch:
+                    return "OK"
+                return "l"
+
+            def qLaunchSuccess(self):
+                return "OK"
+
+            def qProcessInfo(self):
+                return "$pid:10b70;parent-pid:10b20;real-uid:1f6;real-gid:14;effective-uid:1f6;effective-gid:14;cputype:1000007;cpusubtype:8;ptrsize:8;ostype:macosx;vendor:apple;endian:little;"
+
+            
+        error = lldb.SBError()
+        self.server.responder = MyResponder(self)
+        target = self.dbg.CreateTarget(None, "x86_64-apple-macosx", "remote-macosx", False, error)
+        self.assertSuccess(error, "Made a valid target")
+        launch_info = target.GetLaunchInfo()
+        launch_info.SetExecutableFile(lldb.SBFileSpec(self.absent_file), True)
+        flags = launch_info.GetLaunchFlags()
+        flags |= lldb.eLaunchFlagStopAtEntry
+        launch_info.SetLaunchFlags(flags)
+
+        process = self.connect(target)
+        self.assertTrue(process.IsValid(), "Process is valid")
+
+        # We need to fetch the connected event:
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected])
+
+        self.server.responder.after_launch = True
+
+        process = target.Launch(launch_info, error)
+        self.assertSuccess(error, "Successfully launched.")
+        self.assertEqual(process.GetState(), lldb.eStateStopped, "Should be stopped at entry")
+        self.assertIsNotNone(self.a_packet_file, "A packet was sent")
+        self.assertEqual(self.absent_file, self.a_packet_file, "The A packet file was correct")