[lldb/Target] Initialize new targets environment variables from target.env-vars

[friss]

Summary

The TargetProperties constructor invokes a series of callbacks to
prime the properties from the default ones. The one callback in
charge of updating the inferior environment was commented out
because it crashed.

The reason for the crash is that TargetProperties is a parent class
of Target and the callbacks were invoked using a Target that was
not fully initialized. This patch moves the initial callback
invocations to a separate function that can be called at the end
the Target constructor, thus preventing the crash.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

The added test checks that the LaunchInfo object returned by
the target has been primed with the values from the settings.

[labath]

Another option might be to make TargetProperties a member instead of a base class. I don't think the current setup makes for a particularly clean design. If TargetProperties is among the last Target members being initialized, then calling the callbacks from the constructor should "just work".

lldb/include/lldb/Target/Target.h

214

This name is very nondescript. I think it would be better to name this according to what it does, instead of how it does it. LoadDefaultPropertyValues() ?

[labath]

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

[friss]

In D76009#1919103, @labath wrote:

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Just to be clear, in that test the host environment was modified between the creation of the target and the launch which I think is pretty unlikely to matter in practice.

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

The inherited environment is collected the first time the property is accessed and running the callback triggers this. For some reason just doing set show target.env-vars doesn't trigger it though, I'd need to debug this to understand. So yes, with this change, the target.inherit-env setting will take effect at target creation time and not at whatever next point in time the lazy logic is triggered.

One way to fix this is to add another callback for the target.inherit-env property and remove the variables that are in the environment.

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

Jim would certainly be able to explain the intent here.

[jingham]

In D76009#1919554, @friss wrote:

In D76009#1919103, @labath wrote:

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Just to be clear, in that test the host environment was modified between the creation of the target and the launch which I think is pretty unlikely to matter in practice.

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

The inherited environment is collected the first time the property is accessed and running the callback triggers this. For some reason just doing set show target.env-vars doesn't trigger it though, I'd need to debug this to understand. So yes, with this change, the target.inherit-env setting will take effect at target creation time and not at whatever next point in time the lazy logic is triggered.

One way to fix this is to add another callback for the target.inherit-env property and remove the variables that are in the environment.

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

Jim would certainly be able to explain the intent here.

I don't think the current version of the code works very well. You can actually only change the value of target.inherit-env BEFORE the first run. For instance:

 > lldb printenv
warning: Overwriting existing definition for 'p'.
warning: Overwriting existing definition for 't'.
(lldb) target create "printenv"
Current executable set to 'printenv' (x86_64).
(lldb) set set target.in
Available completions:
	target.inherit-env               
	target.input-path                
	target.inline-breakpoint-strategy
(lldb) set set target.inherit-env 0
(lldb) set show target.env-vars
target.env-vars (dictionary of strings) =
(lldb) run
Process 65813 launched: '/tmp/printenv' (x86_64)
0  :OS_ACTIVITY_DT_MODE=enable:
Process 65813 exited with status = 0 (0x00000000) 
(lldb) set set target.inherit-env 1
(lldb) run
Process 65816 launched: '/tmp/printenv' (x86_64)
0  :OS_ACTIVITY_DT_MODE=enable:
Process 65816 exited with status = 0 (0x00000000)

lldb had way more environment variables than that...

And conversely, if you run with inherit-env set to 1 and then set it to 0, you still get all the environment variables.

For this to behave correctly, we would as Fred suggests have to add a callback to setting inherit-env and add or remove the host's variables as appropriate when it changes.

IMO there should really also be a "target create" option for inherit-env. This is usually something you want to do depending on the type of target, and you don't generally change it after the fact. It's awkward to have to change a setting to create this or that target in a certain mode. It would be better if we had predicates for the settings, then you could do something like settings set target[platform=remote].inherit-env 0. But we haven't gotten there yet...

Note, for this to be really useful for remote debugging, lldb should be able to fetch the monitor's environment and use that to prime the target's environment when inherit-env is on. That would be a much more useful thing to do. But that's way outside the scope of this patch.

[jingham]

BTW, don't worry about the OS_ACTIVITY_DT_MODE env var. That's something you have to set when debugging or the Foundation loggging method (NSLog) will go to the system console not to the current terminal, so we force it on all processes on macOS...

[friss]

I put up a "fix" for the inherit-env issue mentioned here: https://reviews.llvm.org/D76105
It is mostly orthogonal to this patch as Jim showed the behavior today is already broken.

[labath]

In D76009#1920548, @friss wrote:

I put up a "fix" for the inherit-env issue mentioned here: https://reviews.llvm.org/D76105
It is mostly orthogonal to this patch as Jim showed the behavior today is already broken.

Yeah, I agree the current behavior is pretty bad, and this patch at least cleans up one aspect of it.

I don't have an immediate use case for the current env-vars behavior. If this helps you in any way, then I don't want to block it just because of that.

[friss]

Make helper function name more descriptive.

lldb/include/lldb/Target/Target.h

public:
   void SetInjectLocalVariables(ExecutionContext *exe_ctx, bool b);
 
   void SetRequireHardwareBreakpoints(bool b);
 
   bool GetRequireHardwareBreakpoints() const;
 
   bool GetAutoInstallMainExecutable() const;
 
+  void UpdateLaunchInfoFromProperties();
+
 private:
   // Callbacks for m_launch_info.
   void Arg0ValueChangedCallback();
   void RunArgsValueChangedCallback();
   void EnvVarsValueChangedCallback();
   void InputPathValueChangedCallback();
   void OutputPathValueChangedCallback();
   void ErrorPathValueChangedCallback();

lldb/source/Target/Target.cpp

Target::Target(Debugger &debugger, const ArchSpec &target_arch,
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT),
            "{0} Target::Target()", static_cast<void *>(this));
   if (target_arch.IsValid()) {
     LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET),
              "Target::Target created with architecture {0} ({1})",
              target_arch.GetArchitectureName(),
              target_arch.GetTriple().getTriple().c_str());
   }
+
+  UpdateLaunchInfoFromProperties();
 }
 
 Target::~Target() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   LLDB_LOG(log, "{0} Target::~Target()", static_cast<void *>(this));
   DeleteCurrentProcess();
 }
 
m_collection_sp->SetValueChangedCallback(
         ePropertyDisableSTDIO, [this] { DisableSTDIOValueChangedCallback(); });
 
     m_experimental_properties_up.reset(new TargetExperimentalProperties());
     m_collection_sp->AppendProperty(
         ConstString(Properties::GetExperimentalSettingsName()),
         ConstString("Experimental settings - setting these won't produce "
                     "errors if the setting is not present."),
         true, m_experimental_properties_up->GetValueProperties());
-
-    // Update m_launch_info once it was created
-    Arg0ValueChangedCallback();
-    RunArgsValueChangedCallback();
-    // EnvVarsValueChangedCallback(); // FIXME: cause segfault in
-    // Target::GetPlatform()
-    InputPathValueChangedCallback();
-    OutputPathValueChangedCallback();
-    ErrorPathValueChangedCallback();
-    DetachOnErrorValueChangedCallback();
-    DisableASLRValueChangedCallback();
-    DisableSTDIOValueChangedCallback();
   } else {
     m_collection_sp =
         std::make_shared<TargetOptionValueProperties>(ConstString("target"));
     m_collection_sp->Initialize(g_target_properties);
     m_experimental_properties_up.reset(new TargetExperimentalProperties());
     m_collection_sp->AppendProperty(
         ConstString(Properties::GetExperimentalSettingsName()),
         ConstString("Experimental settings - setting these won't produce "
                     "errors if the setting is not present."),
         true, m_experimental_properties_up->GetValueProperties());
     m_collection_sp->AppendProperty(
         ConstString("process"), ConstString("Settings specific to processes."),
         true, Process::GetGlobalProperties()->GetValueProperties());
   }
 }
 
 TargetProperties::~TargetProperties() = default;
 
+void TargetProperties::UpdateLaunchInfoFromProperties() {
+  Arg0ValueChangedCallback();
+  RunArgsValueChangedCallback();
+  EnvVarsValueChangedCallback();
+  InputPathValueChangedCallback();
+  OutputPathValueChangedCallback();
+  ErrorPathValueChangedCallback();
+  DetachOnErrorValueChangedCallback();
+  DisableASLRValueChangedCallback();
+  DisableSTDIOValueChangedCallback();
+}
+
 bool TargetProperties::GetInjectLocalVariables(
     ExecutionContext *exe_ctx) const {
   const Property *exp_property = m_collection_sp->GetPropertyAtIndex(
       exe_ctx, false, ePropertyExperimental);
   OptionValueProperties *exp_values =
       exp_property->GetValue()->GetAsProperties();
   if (exp_values)
     return exp_values->GetPropertyAtIndexAsBoolean(

lldb/test/API/commands/settings/TestSettings.py

def test_run_args_and_env_vars(self):
         # And add hooks to restore the settings during tearDown().
         self.runCmd('settings set target.run-args A B C')
         self.addTearDownHook(
             lambda: self.runCmd("settings clear target.run-args"))
         self.runCmd('settings set target.env-vars ["MY_ENV_VAR"]=YES')
         self.addTearDownHook(
             lambda: self.runCmd("settings clear target.env-vars"))
 
+        launch_info = self.dbg.GetTargetAtIndex(0).GetLaunchInfo()
+        found_env_var = False
+        for i in range(0, launch_info.GetNumEnvironmentEntries()):
+            if launch_info.GetEnvironmentEntryAtIndex(i) == "MY_ENV_VAR=YES":
+                found_env_var = True
+                break
+        self.assertTrue(found_env_var,
+                        "MY_ENV_VAR was not set in LunchInfo object")
+
         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, "output2.txt")
 
         self.expect(
             output,
             exe=False,
             substrs=[
                 "argv[1] matches",
                 "argv[2] matches",
                 "argv[3] matches",
                 "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."""
         self.build()
 
-        exe = self.getBuildArtifact("a.out")
-        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
-
-        # By default, inherit-env is 'true'.
-        self.expect(
-            'settings show target.inherit-env',
-            "Default inherit-env is 'true'",
-            startstr="target.inherit-env (boolean) = true")
-
         # Set some host environment variables now.
         os.environ["MY_HOST_ENV_VAR1"] = "VAR1"
         os.environ["MY_HOST_ENV_VAR2"] = "VAR2"
 
         # This is the function to unset the two env variables set above.
         def unset_env_variables():
             os.environ.pop("MY_HOST_ENV_VAR1")
             os.environ.pop("MY_HOST_ENV_VAR2")
 
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        # By default, inherit-env is 'true'.
+        self.expect(
+            'settings show target.inherit-env',
+            "Default inherit-env is 'true'",
+            startstr="target.inherit-env (boolean) = true")
+
         self.addTearDownHook(unset_env_variables)
         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(