[Lldb-commits] [PATCH] D115246: [lldb/qemu] Separate host and target environments

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp:121
+    auto host_it = host.find(KV.first());
+    if (host_it == host.end() || host_it->second != KV.second)
+      set_env.push_back(Environment::compose(KV));
----------------
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.


================
Comment at: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp:127
+  for (const auto &KV : host) {
+    if (target.count(KV.first()) == 0)
+      unset_env.push_back(KV.first());
----------------
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.


================
Comment at: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td:18
+    ElementType<"String">,
+    Desc<"Extra variables to add to target environment.">;
 }
----------------
"to emulated target environment", just to be super clear?


================
Comment at: lldb/test/API/qemu/qemu.py:65
+    state = vars(args)
+    state["environ"] = dict(os.environ)
+    emulator = FakeEmulator(args.g, state)
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115246/new/

https://reviews.llvm.org/D115246