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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
