[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: tools/debugserver/source/debugserver.cpp:1426
+    for (i = 0; (env_entry = host_env[i]) != NULL; ++i)
+      remote->Context().PushEnvironment(env_entry);
+  }
----------------
clayborg wrote:
> We need to check if the env var is already in the environment in remote->Context() first before pushing the new version. I would assume that if you exec a program with an env like:
> ```
> FOO=bar
> USER=me
> FOO=baz
> ```
> 
> That FOO=baz will end up being the value that is used. If the user specifies things with --env, we will just overwrite it. We might add a PushEnvironmentIfNeeded() function to the Context() class that will make sure it isn't in the list first and push it only if needed.
That isn't what happens. The execve() will just forward both definitions (you can see this by exec-ing /usr/bin/env, which will print both), but getenv() will return the first one.  (at least on darwin and linux, but I expect others to behave the same).

I tried this out as while working on D41359, I got the impression that there is some confusion about what the code expected to happen when it called env.AppendArguments() -- e.g. should `SBLaunchInfo::SetEnvironmentEntries(..., append=true)` overwrite the existing entries or not? Based on these experiments, I replaced AppendArguments call with `env.insert(...)` (which does not overwrite), but it's possible some of these should actually by replaced by `insert_or_assign`..

That said, I agree that relying on this behavior here is dodgy. I'll see if I can remove it.


================
Comment at: unittests/tools/lldb-server/tests/LLGSTest.cpp:35
+  // Test that --env takes precedence over inherited environment variables.
+  putenv(const_cast<char *>("LLDB_TEST_MAGIC_VARIABLE=foobar"));
+
----------------
This test shows that the `--env` below wins over this value from the parent environment (the test inferior will check it's own environment via getenv and report the result via exit status)


https://reviews.llvm.org/D41352