jingham added a comment. 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. 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, all the "host side" work uses the return from GetTarget().GetExecutableModulePointer() returns null. That's always going to be true in the scenario I'm adding support for. So we won't look at the local file system again till 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... 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
