Author: Fred Riss Date: 2020-03-23T07:58:34-07:00 New Revision: b4a6e63ea12309bf667d1569a20ec5b081cbf2a4
URL: https://github.com/llvm/llvm-project/commit/b4a6e63ea12309bf667d1569a20ec5b081cbf2a4 DIFF: https://github.com/llvm/llvm-project/commit/b4a6e63ea12309bf667d1569a20ec5b081cbf2a4.diff LOG: [lldb/Target] Rework the way the inferior environment is created Summary: The interactions between the environment settings (`target.env-vars`, `target.inherit-env`) and the inferior life-cycle are non-obvious today. For example, if `target.inherit-env` is set, the `target.env-vars` setting will be augmented with the contents of the host environment the first time the launch environment is queried (usually at launch). After that point, toggling `target.inherit-env` will have no effect as there's no tracking of what comes from the host and what is a user setting. This patch computes the environment every time it is queried rather than updating the contents of the `target.env-vars` property. This means that toggling the `target.inherit-env` property later will now have the intended effect. This patch also adds a `target.unset-env-vars` settings that one can use to remove variables from the launch environment. Using this, you can inherit all but a few of the host environment. The way the launch environment is constructed is: 1/ if `target.inherit-env` is set, then read the host environment into the launch environment. 2/ Remove for the environment the variables listed in `target.unset-env`. 3/ Augment the launch environment with the contents of `target.env-vars`. This overrides any common values with the host environment. The one functional difference here that could be seen as a regression is that `target.env-vars` will not contain the inferior environment after launch. The patch implements a better alternative in the `target show-launch-environment` command which will return the environment computed through the above rules. Reviewers: labath, jingham Subscribers: lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D76470 Added: Modified: lldb/include/lldb/Target/Target.h lldb/source/Commands/CommandObjectTarget.cpp lldb/source/Target/Target.cpp lldb/source/Target/TargetProperties.td lldb/test/API/commands/settings/TestSettings.py Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index 2e7932f49e6f..77cda4998192 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -225,9 +225,12 @@ class TargetProperties : public Properties { void DisableASLRValueChangedCallback(); void DisableSTDIOValueChangedCallback(); + Environment ComputeEnvironment() const; + // Member variables. ProcessLaunchInfo m_launch_info; std::unique_ptr<TargetExperimentalProperties> m_experimental_properties_up; + Target *m_target; }; class EvaluateExpressionOptions { diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp index c70117c7a80a..95f81fc6cd54 100644 --- a/lldb/source/Commands/CommandObjectTarget.cpp +++ b/lldb/source/Commands/CommandObjectTarget.cpp @@ -682,6 +682,41 @@ class CommandObjectTargetDelete : public CommandObjectParsed { OptionGroupBoolean m_cleanup_option; }; +class CommandObjectTargetShowLaunchEnvironment : public CommandObjectParsed { +public: + CommandObjectTargetShowLaunchEnvironment(CommandInterpreter &interpreter) + : CommandObjectParsed( + interpreter, "target show-launch-environment", + "Shows the environment being passed to the process when launched, " + "taking info account 3 settings: target.env-vars, " + "target.inherit-env and target.unset-env-vars.", + nullptr, eCommandRequiresTarget) {} + + ~CommandObjectTargetShowLaunchEnvironment() override = default; + +protected: + bool DoExecute(Args &args, CommandReturnObject &result) override { + Target *target = m_exe_ctx.GetTargetPtr(); + Environment env = target->GetEnvironment(); + + std::vector<Environment::value_type *> env_vector; + env_vector.reserve(env.size()); + for (auto &KV : env) + env_vector.push_back(&KV); + std::sort(env_vector.begin(), env_vector.end(), + [](Environment::value_type *a, Environment::value_type *b) { + return a->first() < b->first(); + }); + + auto &strm = result.GetOutputStream(); + for (auto &KV : env_vector) + strm.Format("{0}={1}\n", KV->first(), KV->second); + + result.SetStatus(eReturnStatusSuccessFinishResult); + return result.Succeeded(); + } +}; + #pragma mark CommandObjectTargetVariable // "target variable" @@ -4876,6 +4911,9 @@ CommandObjectMultiwordTarget::CommandObjectMultiwordTarget( CommandObjectSP(new CommandObjectTargetList(interpreter))); LoadSubCommand("select", CommandObjectSP(new CommandObjectTargetSelect(interpreter))); + LoadSubCommand("show-launch-environment", + CommandObjectSP(new CommandObjectTargetShowLaunchEnvironment( + interpreter))); LoadSubCommand( "stop-hook", CommandObjectSP(new CommandObjectMultiwordTargetStopHooks(interpreter))); diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 0162ca838e78..e2c808120877 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -3344,16 +3344,13 @@ enum { class TargetOptionValueProperties : public OptionValueProperties { public: - TargetOptionValueProperties(ConstString name) - : OptionValueProperties(name), m_target(nullptr), m_got_host_env(false) {} + TargetOptionValueProperties(ConstString name) : OptionValueProperties(name) {} // This constructor is used when creating TargetOptionValueProperties when it // is part of a new lldb_private::Target instance. It will copy all current // global property values as needed - TargetOptionValueProperties(Target *target, - const TargetPropertiesSP &target_properties_sp) - : OptionValueProperties(*target_properties_sp->GetValueProperties()), - m_target(target), m_got_host_env(false) {} + TargetOptionValueProperties(const TargetPropertiesSP &target_properties_sp) + : OptionValueProperties(*target_properties_sp->GetValueProperties()) {} const Property *GetPropertyAtIndex(const ExecutionContext *exe_ctx, bool will_modify, @@ -3361,9 +3358,6 @@ class TargetOptionValueProperties : public OptionValueProperties { // When getting the value for a key from the target options, we will always // try and grab the setting from the current target if there is one. Else // we just use the one from this instance. - if (idx == ePropertyEnvVars) - GetHostEnvironmentIfNeeded(); - if (exe_ctx) { Target *target = exe_ctx->GetTargetPtr(); if (target) { @@ -3376,41 +3370,6 @@ class TargetOptionValueProperties : public OptionValueProperties { } return ProtectedGetPropertyAtIndex(idx); } - - lldb::TargetSP GetTargetSP() { return m_target->shared_from_this(); } - -protected: - void GetHostEnvironmentIfNeeded() const { - if (!m_got_host_env) { - if (m_target) { - m_got_host_env = true; - const uint32_t idx = ePropertyInheritEnv; - if (GetPropertyAtIndexAsBoolean( - nullptr, idx, g_target_properties[idx].default_uint_value != 0)) { - PlatformSP platform_sp(m_target->GetPlatform()); - if (platform_sp) { - Environment env = platform_sp->GetEnvironment(); - OptionValueDictionary *env_dict = - GetPropertyAtIndexAsOptionValueDictionary(nullptr, - ePropertyEnvVars); - if (env_dict) { - const bool can_replace = false; - for (const auto &KV : env) { - // Don't allow existing keys to be replaced with ones we get - // from the platform environment - env_dict->SetValueForKey( - ConstString(KV.first()), - OptionValueSP(new OptionValueString(KV.second.c_str())), - can_replace); - } - } - } - } - } - } - } - Target *m_target; - mutable bool m_got_host_env; }; // TargetProperties @@ -3437,10 +3396,10 @@ TargetExperimentalProperties::TargetExperimentalProperties() // TargetProperties TargetProperties::TargetProperties(Target *target) - : Properties(), m_launch_info() { + : Properties(), m_launch_info(), m_target(target) { if (target) { m_collection_sp = std::make_shared<TargetOptionValueProperties>( - target, Target::GetGlobalProperties()); + Target::GetGlobalProperties()); // Set callbacks to update launch_info whenever "settins set" updated any // of these properties @@ -3450,6 +3409,10 @@ TargetProperties::TargetProperties(Target *target) ePropertyRunArgs, [this] { RunArgsValueChangedCallback(); }); m_collection_sp->SetValueChangedCallback( ePropertyEnvVars, [this] { EnvVarsValueChangedCallback(); }); + m_collection_sp->SetValueChangedCallback( + ePropertyUnsetEnvVars, [this] { EnvVarsValueChangedCallback(); }); + m_collection_sp->SetValueChangedCallback( + ePropertyInheritEnv, [this] { EnvVarsValueChangedCallback(); }); m_collection_sp->SetValueChangedCallback( ePropertyInputPath, [this] { InputPathValueChangedCallback(); }); m_collection_sp->SetValueChangedCallback( @@ -3641,19 +3604,43 @@ void TargetProperties::SetRunArguments(const Args &args) { m_launch_info.GetArguments() = args; } +Environment TargetProperties::ComputeEnvironment() const { + Environment env; + + if (m_target && + m_collection_sp->GetPropertyAtIndexAsBoolean( + nullptr, ePropertyInheritEnv, + g_target_properties[ePropertyInheritEnv].default_uint_value != 0)) { + if (auto platform_sp = m_target->GetPlatform()) { + Environment platform_env = platform_sp->GetEnvironment(); + for (const auto &KV : platform_env) + env[KV.first()] = KV.second; + } + } + + Args property_unset_env; + m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyUnsetEnvVars, + property_unset_env); + for (const auto &var : property_unset_env) + env.erase(var.ref()); + + Args property_env; + m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyEnvVars, + property_env); + for (const auto &KV : Environment(property_env)) + env[KV.first()] = KV.second; + + return env; +} + Environment TargetProperties::GetEnvironment() const { - // TODO: Get rid of the Args intermediate step - Args env; - const uint32_t idx = ePropertyEnvVars; - m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, idx, env); - return Environment(env); + return ComputeEnvironment(); } void TargetProperties::SetEnvironment(Environment env) { // TODO: Get rid of the Args intermediate step const uint32_t idx = ePropertyEnvVars; m_collection_sp->SetPropertyAtIndexFromArgs(nullptr, idx, Args(env)); - m_launch_info.GetEnvironment() = std::move(env); } bool TargetProperties::GetSkipPrologue() const { @@ -3971,7 +3958,7 @@ void TargetProperties::RunArgsValueChangedCallback() { } void TargetProperties::EnvVarsValueChangedCallback() { - m_launch_info.GetEnvironment() = GetEnvironment(); + m_launch_info.GetEnvironment() = ComputeEnvironment(); } void TargetProperties::InputPathValueChangedCallback() { diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td index 77579c66ccb0..c8dd0a12315e 100644 --- a/lldb/source/Target/TargetProperties.td +++ b/lldb/source/Target/TargetProperties.td @@ -80,7 +80,10 @@ let Definition = "target" in { Desc<"A list containing all the arguments to be passed to the executable when it is run. Note that this does NOT include the argv[0] which is in target.arg0.">; def EnvVars: Property<"env-vars", "Dictionary">, ElementType<"String">, - Desc<"A list of all the environment variables to be passed to the executable's environment, and their values.">; + Desc<"A list of user provided environment variables to be passed to the executable's environment, and their values.">; + def UnsetEnvVars: Property<"unset-env-vars", "Array">, + ElementType<"String">, + Desc<"A list of environment variable names to be unset in the inferior's environment. This is most useful to unset some host environment variables when target.inherit-env is true. target.env-vars takes precedence over target.unset-env-vars.">; def InheritEnv: Property<"inherit-env", "Boolean">, DefaultTrue, Desc<"Inherit the environment from the process that is running LLDB.">; diff --git a/lldb/test/API/commands/settings/TestSettings.py b/lldb/test/API/commands/settings/TestSettings.py index b203f85de9fb..29360856a735 100644 --- a/lldb/test/API/commands/settings/TestSettings.py +++ b/lldb/test/API/commands/settings/TestSettings.py @@ -236,6 +236,10 @@ def do_test_run_args_and_env_vars(self, use_launchsimple): self.assertTrue(found_env_var, "MY_ENV_VAR was not set in LunchInfo object") + self.expect( + 'target show-launch-environment', + substrs=["MY_ENV_VAR=YES"]) + wd = self.get_process_working_directory() if use_launchsimple: process = target.LaunchSimple(None, None, wd) @@ -256,6 +260,31 @@ def do_test_run_args_and_env_vars(self, use_launchsimple): "argv[3] matches", "Environment variable 'MY_ENV_VAR' successfully passed."]) + # Check that env-vars overrides unset-env-vars. + self.runCmd('settings set target.unset-env-vars MY_ENV_VAR') + + self.expect( + 'target show-launch-environment', + 'env-vars overrides unset-env-vars', + substrs=["MY_ENV_VAR=YES"]) + + wd = self.get_process_working_directory() + if use_launchsimple: + process = target.LaunchSimple(None, None, wd) + self.assertTrue(process) + else: + self.runCmd("process launch --working-dir '{0}'".format(wd), + RUN_SUCCEEDED) + + # Read the output file produced by running the program. + output = lldbutil.read_file_from_process_wd(self, "output2.txt") + + self.expect( + output, + exe=False, + substrs=[ + "Environment variable 'MY_ENV_VAR' successfully passed."]) + @skipIfRemote # it doesn't make sense to send host env to remote target def test_pass_host_env_vars(self): """Test that the host env vars are passed to the launched process.""" @@ -269,6 +298,7 @@ def test_pass_host_env_vars(self): def unset_env_variables(): os.environ.pop("MY_HOST_ENV_VAR1") os.environ.pop("MY_HOST_ENV_VAR2") + self.addTearDownHook(unset_env_variables) exe = self.getBuildArtifact("a.out") self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) @@ -279,7 +309,33 @@ def unset_env_variables(): "Default inherit-env is 'true'", startstr="target.inherit-env (boolean) = true") - self.addTearDownHook(unset_env_variables) + self.expect( + 'target show-launch-environment', + 'Host environment is passed correctly', + substrs=['MY_HOST_ENV_VAR1=VAR1', 'MY_HOST_ENV_VAR2=VAR2']) + self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()), + RUN_SUCCEEDED) + + # Read the output file produced by running the program. + output = lldbutil.read_file_from_process_wd(self, "output1.txt") + + self.expect( + output, + exe=False, + substrs=[ + "The host environment variable 'MY_HOST_ENV_VAR1' successfully passed.", + "The host environment variable 'MY_HOST_ENV_VAR2' successfully passed."]) + + # Now test that we can prevent the inferior from inheriting the + # environment. + self.runCmd('settings set target.inherit-env false') + + self.expect( + 'target show-launch-environment', + 'target.inherit-env affects `target show-launch-environment`', + matching=False, + substrs = ['MY_HOST_ENV_VAR1=VAR1', 'MY_HOST_ENV_VAR2=VAR2']) + self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()), RUN_SUCCEEDED) @@ -289,10 +345,42 @@ def unset_env_variables(): self.expect( output, exe=False, + matching=False, substrs=[ "The host environment variable 'MY_HOST_ENV_VAR1' successfully passed.", "The host environment variable 'MY_HOST_ENV_VAR2' successfully passed."]) + # Now test that we can unset variables from the inherited environment. + self.runCmd('settings set target.inherit-env true') + self.runCmd('settings set target.unset-env-vars MY_HOST_ENV_VAR1') + self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()), + RUN_SUCCEEDED) + + # Read the output file produced by running the program. + output = lldbutil.read_file_from_process_wd(self, "output1.txt") + + self.expect( + 'target show-launch-environment', + 'MY_HOST_ENV_VAR1 is unset, it shouldn\'t be in `target show-launch-environment`', + matching=False, + substrs = ['MY_HOST_ENV_VAR1=VAR1']) + self.expect( + 'target show-launch-environment', + 'MY_HOST_ENV_VAR2 shouldn be in `target show-launch-environment`', + substrs = ['MY_HOST_ENV_VAR2=VAR2']) + + self.expect( + output, + exe=False, + matching=False, + substrs=[ + "The host environment variable 'MY_HOST_ENV_VAR1' successfully passed."]) + self.expect( + output, + exe=False, + substrs=[ + "The host environment variable 'MY_HOST_ENV_VAR2' successfully passed."]) + @skipIfDarwinEmbedded # <rdar://problem/34446098> debugserver on ios etc can't write files def test_set_error_output_path(self): """Test that setting target.error/output-path for the launched process works.""" _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
