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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
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());
----------------
DavidSpickett wrote:
> 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.
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115246