The new testing for “inherit-env=false” is failing on Windows. I skipped the test for now.
Could it be that it never worked there? (In which case XFail would be a better resolution) Does anyone have easy access to a Windows build to try it out? Thanks, Fred > On Mar 23, 2020, at 7:59 AM, Fred Riss via lldb-commits > <lldb-commits@lists.llvm.org> wrote: > > > 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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
