[Swift REPL] Inherit the environment for the Swift REPL #9853
Conversation
|
@swift-ci test |
| @@ -188,23 +188,16 @@ lldb::REPLSP SwiftREPL::CreateInstanceFromDebugger(Status &err, | |||
| // breakpoint above, it better | |||
| // say it is internal | |||
|
|
|||
| lldb_private::ProcessLaunchInfo launch_info; | |||
| lldb_private::ProcessLaunchInfo launch_info = | |||
| target_sp->GetProcessLaunchInfo(); | |||
| llvm::StringRef target_settings_argv0 = target_sp->GetArg0(); | |||
|
|
||
| if (target_sp->GetDisableSTDIO()) | ||
| launch_info.GetFlags().Set(eLaunchFlagDisableSTDIO); | ||
|
|
||
| if (!target_settings_argv0.empty()) { | ||
| launch_info.GetArguments().AppendArgument(target_settings_argv0); | ||
| launch_info.SetExecutableFile(exe_module_sp->GetPlatformFileSpec(), false); | ||
| } else { | ||
| launch_info.SetExecutableFile(exe_module_sp->GetPlatformFileSpec(), true); | ||
| } | ||
|
|
There was a problem hiding this comment.
If you get the LaunchEnvironment from the target, these should also be properly set already. I don't think you want to override them here either. The benefit of this change is it allows you to set everything you need to set for the REPL execution in the Target you make for it, then we don't have to overwrite anything here.
There was a problem hiding this comment.
I thought the same thing, but All the REPL test fail without this code. I didn't want to get sidetracked so I left a FIXME to figure out why this is necessary and document it.
There was a problem hiding this comment.
Okay. Looking more closely, if you have set up the arguments properly in the Target, we won't muck with them, so I guess having a "If you don't, we'll fix it for you..." is okay for now.
JDevlieghere
force-pushed
the
rdar137522456
branch
from
eb184f1 to
859eab8
Compare
The Swift REPL was had rolling the ProcessLaunchInfo instead of using the one already setup by the target. While populating the launch info, it used `Target::GetTargetEnvironment()` which only looks at the corresponding setting, and ignoring the `inherit-environment` setting. The alternative of calling `Target::GetInheritedEnvironment` is also wrong, because that has the inverse problem: it only inherits the environment and doesn't merge it with the target's environment variables. The solution is to use `Target::GetProcessLaunchInfo` which populates the launch info correctly. Propagating the environment correctly is particularly important on Windows where `Path` needs to be passed to the inferior to allow `LoadLibraryW(L"swiftCore.dll")` to succeed. Without this patch, the library is not found and the inferior exits terminating the REPL instance. - Fixes: llvm#9551 - Will fix swiftlang/swift#76702 and swiftlang/swift#70005 with a corresponding driver change to set `inherit-environment` when invoking the REPL. rdar://137522456
JDevlieghere
force-pushed
the
rdar137522456
branch
from
859eab8 to
79d7505
Compare
|
@swift-ci test |
|
|
||
| if (target_sp->GetDisableSTDIO()) | ||
| launch_info.GetFlags().Set(eLaunchFlagDisableSTDIO); | ||
|
|
||
| if (!target_settings_argv0.empty()) { | ||
| launch_info.GetArguments().AppendArgument(target_settings_argv0); | ||
| launch_info.SetExecutableFile(exe_module_sp->GetPlatformFileSpec(), false); | ||
| } else { | ||
| launch_info.SetExecutableFile(exe_module_sp->GetPlatformFileSpec(), true); | ||
| } | ||
|
|
There was a problem hiding this comment.
Okay. Looking more closely, if you have set up the arguments properly in the Target, we won't muck with them, so I guess having a "If you don't, we'll fix it for you..." is okay for now.
The Swift REPL was had rolling the ProcessLaunchInfo instead of using the one already setup by the target. While populating the launch info, it used
Target::GetTargetEnvironment()which only looks at the corresponding setting, and ignoring theinherit-environmentsetting. The alternative of callingTarget::GetInheritedEnvironmentis also wrong, because that has the inverse problem: it only inherits the environment and doesn't merge it with the target's environment variables. The solution is to useTarget::GetProcessLaunchInfowhich populates the launch info correctly.Propagating the environment correctly is particularly important on Windows where
Pathneeds to be passed to the inferior to allowLoadLibraryW(L"swiftCore.dll")to succeed. Without this patch, the library is not found and the inferior exits terminating the REPL instance.Fixes: swiftlang/swift#76702
Fixes: swiftlang/swift#70005
Fixes: #9551
rdar://137522456