[Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created
Frédéric Riss via lldb-commits
lldb-commits at lists.llvm.org
Mon Mar 23 09:17:46 PDT 2020
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 at 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 at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
More information about the lldb-commits
mailing list