[lldb/qemu] Separate host and target environments

[labath]

Summary

Qemu normally forwards its (host) environment variables to the emulated
process. While this works fine for most variables, there are some (few, but
fairly important) variables where this is not possible. LD_LIBRARY_PATH
is the probably the most important of those -- we don't want the library
search path for the emulated libraries to interfere with the libraries
that the emulator itself needs.

For this reason, qemu provides a mechanism (QEMU_SET_ENV,
QEMU_UNSET_ENV) to set variables only for the emulated process. This
patch makes use of that functionality to pass any user-provided
variables to the emulated process. Since we're piggy-backing on the
normal lldb environment-handling mechanism, all the usual mechanism to
provide environment (target.env-vars setting, SBLaunchInfo, etc.) work
out-of-the-box, and the only thing we need to do is to properly
construct the qemu environment variables.

This patch also adds a new setting -- target-env-vars, which represents
environment variables which are added (on top of the host environment)
to the default launch environments of all (qemu) targets. The reason for
its existence is to enable the configuration (e.g., from a startup
script) of the default launch environment, before any target is created.
The idea is that this would contain the variables (like the
aforementioned LD_LIBRARY_PATH) common to all targets being debugged on
the given system. The user is, of course, free to customize the
environment for a particular target in the usual manner.

The reason I do not want to use/recommend the "global" version of the
target.env-vars setting for this purpose is that the setting would apply
to all targets, whereas the settings (their values) I have mentioned
would be specific to the given platform.

[DavidSpickett]

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

121

Please comment with the meaning here. Which I think is if this var is not set for the host, or it is set but to a different value.

127

And here you're saying if the setting is set for host but not target, unset it?

And then I thought doesn't this just leave you with the content of target but that's not the goal here. You start with:
target environment
host environment

You want:
host environment plus QEMU_SET/UNSET_ENV based on the target environment's content

Since you might still need some of the vars that get unset, before the point where qemu's emulation begins, right? So maybe a bad example idk, but you might want LD_LIBRARY_PATH set to one thing to run qemu properly, but then you want it to be changed when qemu does its emulation.

Which is why you can't just use the target environment and discard the host completely.

Could you add a motivating example as a comment to the function? I think it's quite clearly written but easy to come to that incorrect conclusion on first read.

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td

18

"to emulated target environment", just to be super clear?

lldb/test/API/qemu/qemu.py

65

Do you do this so that you know you have a copy, or because os.environ is not quite acting like a dictionary enough?

A quick check shows it does set/get key but I'm sure it's not 100% like a plain dict somehow.

If it's to ensure a copy, not a reference to the object please note that in a comment.

[DavidSpickett]

Oh and:

This patch also adds a new setting -- target-env-vars

Might want to clarify that it adds this setting to the qemu user platform.

[labath]

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

127

Yes, that's the general idea, though it's not that the target environment "replaces" the host one after qemu is "done" with it. It's more like qemu maintaining an emulated version of the environment for the benefit of the emulated program (well, the program maintains it itself, qemu just passes the appropriate data structures when it launches it).

I've also refactored the function to be more... functional. I originally thought I would be doing some in-place modifications of the target environment, but with the current implementation, it just looked confusing.

lldb/test/API/qemu/qemu.py

65

It's mainly because the json library is not able to serialize the magic object. Having a copy is kinda nice, as I do not intend to modify the actual environment, but it does not really matter.

[labath]

• add some explanatory comments
• make the function return a new environment

[DavidSpickett]

LGTM assuming that qemu passing on the SET/UNSET vars is what you've observed.

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

139

I glossed over this the first time, I assume you've observed this in testing. My first assumption was that qemu wouldn't pass them on but hard to tell from the sources. Probably some use case for it (emulating a second qemu-user?).

[labath]

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

139

I'm not sure what's the use case, but yes, that is how it works. My guess it that it is simply because that is the easiest thing to do, and the user can always unset them himself in case he wants to (most of the time, it doesn't matter).

[labath]

Thanks for all the reviews. I'll probably take a break with this now. I'll need to integrate the existing stuff into our internal infrastructure before adding more features.

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.h

lldb::ProcessSP Attach(ProcessAttachInfo &attach_info, Debugger &debugger,
     status.SetErrorString("Not supported");
     return nullptr;
   }
 
   bool IsConnected() const override { return true; }
 
   void CalculateTrapHandlerSymbolNames() override {}
 
-  Environment GetEnvironment() override { return Host::GetEnvironment(); }
+  Environment GetEnvironment() override;
 
 private:
   static lldb::PlatformSP CreateInstance(bool force, const ArchSpec *arch);
   static void DebuggerInitialize(Debugger &debugger);
 
   PlatformQemuUser() : Platform(/*is_host=*/false) {}
 };
 
 } // namespace lldb_private

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

public:
   }
 
   Args GetEmulatorArgs() {
     Args result;
     m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyEmulatorArgs,
                                               result);
     return result;
   }
+
+  Environment GetTargetEnvVars() {
+    Args args;
+    m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyTargetEnvVars,
+                                              args);
+    return Environment(args);
+  }
 };
 
 static PluginProperties &GetGlobalProperties() {
   static PluginProperties g_settings;
   return g_settings;
 }
 
 llvm::StringRef PlatformQemuUser::GetPluginDescriptionStatic() {
std::vector<ArchSpec> PlatformQemuUser::GetSupportedArchitectures() {
   return {};
 }
 
 static auto get_arg_range(const Args &args) {
   return llvm::make_range(args.GetArgumentArrayRef().begin(),
                           args.GetArgumentArrayRef().end());
 }
 
+// Returns the emulator environment which result in the desired environment
+// being presented to the emulated process. We want to be careful about
+// preserving the host environment, as it may contain entries (LD_LIBRARY_PATH,
+// for example) needed for the operation of the emulator itself.
+static Environment ComputeLaunchEnvironment(Environment target,
+                                            Environment host) {
+  std::vector<std::string> set_env;
+  for (const auto &KV : target) {
+    // If the host value differs from the target (or is unset), then set it
+    // through QEMU_SET_ENV. Identical entries will be forwarded automatically.
+    auto host_it = host.find(KV.first());
+    if (host_it == host.end() || host_it->second != KV.second)
+      set_env.push_back(Environment::compose(KV));
+  }
+
+  std::vector<llvm::StringRef> unset_env;
+  for (const auto &KV : host) {
+    // If the target is missing some host entries, then unset them through
+    // QEMU_UNSET_ENV.
+    if (target.count(KV.first()) == 0)
+      unset_env.push_back(KV.first());
+  }
+
+  // The actual QEMU_(UN)SET_ENV variables should not be forwarded to the
+  // target.
+  if (!set_env.empty()) {
+    host["QEMU_SET_ENV"] = llvm::join(set_env, ",");
+    unset_env.push_back("QEMU_SET_ENV");
+  }
+  if (!unset_env.empty()) {
+    unset_env.push_back("QEMU_UNSET_ENV");
+    host["QEMU_UNSET_ENV"] = llvm::join(unset_env, ",");
+  }
+  return host;
+}
+
 lldb::ProcessSP PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
                                                Debugger &debugger,
                                                Target &target, Status &error) {
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PLATFORM);
 
   std::string qemu = GetGlobalProperties().GetEmulatorPath().GetPath();
 
   llvm::SmallString<0> socket_model, socket_path;
lldb::ProcessSP PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
   args.AppendArgument(launch_info.GetExecutableFile().GetPath());
   for (size_t i = 1; i < launch_info.GetArguments().size(); ++i)
     args.AppendArgument(launch_info.GetArguments()[i].ref());
 
   LLDB_LOG(log, "{0} -> {1}", get_arg_range(launch_info.GetArguments()),
            get_arg_range(args));
 
   launch_info.SetArguments(args, true);
+  launch_info.GetEnvironment() = ComputeLaunchEnvironment(
+      std::move(launch_info.GetEnvironment()), Host::GetEnvironment());
   launch_info.SetLaunchInSeparateProcessGroup(true);
   launch_info.GetFlags().Clear(eLaunchFlagDebug);
   launch_info.SetMonitorProcessCallback(ProcessLaunchInfo::NoOpMonitorCallback,
                                         false);
 
   // This is automatically done for host platform in
   // Target::FinalizeFileActions, but we're not a host platform.
   llvm::Error Err = launch_info.SetUpPtyRedirection();
lldb::ProcessSP PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
   if (launch_info.GetPTY().GetPrimaryFileDescriptor() !=
       PseudoTerminal::invalid_fd)
     process_sp->SetSTDIOFileDescriptor(
         launch_info.GetPTY().ReleasePrimaryFileDescriptor());
 
   process_sp->WaitForProcessToStop(llvm::None, nullptr, false, listener_sp);
   return process_sp;
 }
+
+Environment PlatformQemuUser::GetEnvironment() {
+  Environment env = Host::GetEnvironment();
+  for (const auto &KV : GetGlobalProperties().GetTargetEnvVars())
+    env[KV.first()] = KV.second;
+  return env;
+}

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td

 include "../../../../include/lldb/Core/PropertiesBase.td"
 
 let Definition = "platformqemuuser" in {
   def Architecture: Property<"architecture", "String">,
     Global,
     DefaultStringValue<"">,
     Desc<"Architecture to emulate.">;
   def EmulatorPath: Property<"emulator-path", "FileSpec">,
     Global,
     DefaultStringValue<"">,
     Desc<"Path to the emulator binary.">;
   def EmulatorArgs: Property<"emulator-args", "Args">,
     Global,
     DefaultStringValue<"">,
     Desc<"Extra arguments to pass to the emulator.">;
+  def TargetEnvVars: Property<"target-env-vars", "Dictionary">,
+    ElementType<"String">,
+    Desc<"Extra variables to add to emulated target environment.">;
 }

lldb/test/API/qemu/TestQemuLaunch.py

def setUp(self):
                     sys.path = {path!r}
                     runpy.run_path({source!r}, run_name='__main__')
                     """).format(exec=sys.executable, path=sys.path,
                         source=self.getSourcePath("qemu.py")))
 
         self.set_emulator_setting("architecture", self.getArchitecture())
         self.set_emulator_setting("emulator-path", emulator)
 
-    def _run_and_get_state(self):
+    def _create_target(self):
         self.build()
         exe = self.getBuildArtifact()
 
         # Create a target using our platform
         error = lldb.SBError()
         target = self.dbg.CreateTarget(exe, '', 'qemu-user', False, error)
         self.assertSuccess(error)
         self.assertEqual(target.GetPlatform().GetName(), "qemu-user")
+        return target
+
+    def _run_and_get_state(self, target=None, info=None):
+        if target is None:
+            target = self._create_target()
+
+        if info is None:
+            info = target.GetLaunchInfo()
 
         # "Launch" the process. Our fake qemu implementation will pretend it
         # immediately exited.
-        process = target.LaunchSimple(
-                ["dump:" + self.getBuildArtifact("state.log")], None, None)
+        info.SetArguments(["dump:" + self.getBuildArtifact("state.log")], True)
+        error = lldb.SBError()
+        process = target.Launch(info, error)
+        self.assertSuccess(error)
         self.assertIsNotNone(process)
         self.assertEqual(process.GetState(), lldb.eStateExited)
         self.assertEqual(process.GetExitStatus(), 0x47)
 
         # Verify the qemu invocation parameters.
         with open(self.getBuildArtifact("state.log")) as s:
             return json.load(s)
 
     def test_basic_launch(self):
         state = self._run_and_get_state()
 
         self.assertEqual(state["program"], self.getBuildArtifact())
         self.assertEqual(state["args"],
                 ["dump:" + self.getBuildArtifact("state.log")])
 
     def test_stdio_pty(self):
-        self.build()
-        exe = self.getBuildArtifact()
-
-        # Create a target using our platform
-        error = lldb.SBError()
-        target = self.dbg.CreateTarget(exe, '', 'qemu-user', False, error)
-        self.assertSuccess(error)
+        target = self._create_target()
 
-        info = lldb.SBLaunchInfo([
+        info = target.GetLaunchInfo()
+        info.SetArguments([
             "stdin:stdin",
             "stdout:STDOUT CONTENT\n",
             "stderr:STDERR CONTENT\n",
             "dump:" + self.getBuildArtifact("state.log"),
-            ])
+            ], False)
 
         listener = lldb.SBListener("test_stdio")
         info.SetListener(listener)
 
         self.dbg.SetAsync(True)
+        error = lldb.SBError()
         process = target.Launch(info, error)
         self.assertSuccess(error)
         lldbutil.expect_state_changes(self, listener, process,
                 [lldb.eStateRunning])
 
         process.PutSTDIN("STDIN CONTENT\n")
 
         lldbutil.expect_state_changes(self, listener, process,
def test_stdio_redirect(self):
         with open(self.getBuildArtifact("state.log")) as s:
             state = json.load(s)
         self.assertEqual(state["stdin"], "STDIN CONTENT")
 
     def test_bad_emulator_path(self):
         self.set_emulator_setting("emulator-path",
                 self.getBuildArtifact("nonexistent.file"))
 
-        self.build()
-        exe = self.getBuildArtifact()
-
-        error = lldb.SBError()
-        target = self.dbg.CreateTarget(exe, '', 'qemu-user', False, error)
-        self.assertEqual(target.GetPlatform().GetName(), "qemu-user")
-        self.assertSuccess(error)
+        target = self._create_target()
         info = lldb.SBLaunchInfo([])
+        error = lldb.SBError()
         target.Launch(info, error)
         self.assertTrue(error.Fail())
         self.assertIn("doesn't exist", error.GetCString())
 
     def test_extra_args(self):
         self.set_emulator_setting("emulator-args", "-fake-arg fake-value")
         state = self._run_and_get_state()
 
         self.assertEqual(state["fake-arg"], "fake-value")
+
+    def test_target_env_vars(self):
+        # First clear any global environment to have a clean slate for this test
+        self.runCmd("settings clear target.env-vars")
+        self.runCmd("settings clear target.unset-env-vars")
+
+        def var(i):
+            return "LLDB_TEST_QEMU_VAR%d" % i
+
+        # Set some variables in the host environment.
+        for i in range(4):
+            os.environ[var(i)]="from host"
+        def cleanup():
+            for i in range(4):
+                del os.environ[var(i)]
+        self.addTearDownHook(cleanup)
+
+        # And through the platform setting.
+        self.set_emulator_setting("target-env-vars",
+                "%s='from platform' %s='from platform'" % (var(1), var(2)))
+
+        target = self._create_target()
+        info = target.GetLaunchInfo()
+        env = info.GetEnvironment()
+
+        # Platform settings should trump host values.
+        self.assertEqual(env.Get(var(0)), "from host")
+        self.assertEqual(env.Get(var(1)), "from platform")
+        self.assertEqual(env.Get(var(2)), "from platform")
+        self.assertEqual(env.Get(var(3)), "from host")
+
+        # Finally, make some launch_info specific changes.
+        env.Set(var(2), "from target", overwrite=True)
+        env.Unset(var(3))
+        info.SetEnvironment(env, append=False)
+
+        # Now check everything. Launch info changes should trump everything, but
+        # only for the target environment -- the emulator should still get the
+        # host values.
+        state = self._run_and_get_state(target, info)
+        for i in range(4):
+            self.assertEqual(state["environ"][var(i)], "from host")
+        self.assertEqual(state["environ"]["QEMU_SET_ENV"],
+                "%s=from platform,%s=from target" % (var(1), var(2)))
+        self.assertEqual(state["environ"]["QEMU_UNSET_ENV"],
+                "%s,QEMU_SET_ENV,QEMU_UNSET_ENV" % var(3))

lldb/test/API/qemu/qemu.py

 import argparse
 import socket
 import json
+import os
 import sys
 
 import use_lldb_suite
 from lldbsuite.test.gdbclientutils import *
 
 _description = """\
 Implements a fake qemu for testing purposes. The executable program
 is not actually run. Instead a very basic mock process is presented
def main():
     parser = argparse.ArgumentParser(description=_description,
             formatter_class=argparse.RawDescriptionHelpFormatter)
     parser.add_argument('-g', metavar="unix-socket", required=True)
     parser.add_argument('-fake-arg', dest="fake-arg")
     parser.add_argument('program', help="The program to 'emulate'.")
     parser.add_argument("args", nargs=argparse.REMAINDER)
     args = parser.parse_args()
 
-    emulator = FakeEmulator(args.g, vars(args))
+    state = vars(args)
+    state["environ"] = dict(os.environ)
+    emulator = FakeEmulator(args.g, state)
     emulator.run()
 
 if __name__ == "__main__":
     main()