clayborg added a comment. 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. ================ 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; ---------------- I agree with Pavel here, unless you have a case where you need this to work this way? ================ Comment at: lldb/source/Target/Process.cpp:2496-2497 Module *exe_module = GetTarget().GetExecutableModulePointer(); - if (!exe_module) { - error.SetErrorString("executable module does not exist"); - return error; - } - char local_exec_file_path[PATH_MAX]; char platform_exec_file_path[PATH_MAX]; + ---------------- 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. ================ Comment at: lldb/source/Target/Process.cpp:2515-2516 + // IS no local path. + exe_spec_to_use.GetPath(platform_exec_file_path, + sizeof(platform_exec_file_path)); + } else { ---------------- If we switch to std:string for platform_exec_file_path this becomes: ``` platform_exec_file_path = exe_spec_to_use.GetPath(); ``` ================ Comment at: lldb/source/Target/Process.cpp:2519-2520 + exe_spec_to_use = exe_module->GetFileSpec(); + exe_spec_to_use.GetPath(local_exec_file_path, + sizeof(local_exec_file_path)); + exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path, ---------------- If we switch to std:string for local_exec_file_path this becomes: ``` local_exec_file_path = exe_spec_to_use.GetPath(); ``` ================ 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. ---------------- 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? 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
