[lldb/API] Overwrite variables with SBLaunchInfo::SetEnvironment(append=true)

[labath]

Summary

This function was documented to overwrite entries with D76111, which was
adding a couple of similar functions. However, this function (unlike the
functions added in that patch) was/is not actually overwriting variables

• any pre-existing variables would get ignored.

This behavior does not seem to be intentional. In fact, before the refactor in
D41359, this function could introduce duplicate entries, which could
have very surprising effects both inside lldb and on other applications
(some applications would take the first value, some the second one; in
lldb, attempting to unset a variable could make the second variable
become active, etc.).

Overwriting seems to be the most reasonable behavior here, so change the
code to match documentation.

[labath]

this is the line that motivated this patch -- if one has a LD_LIBRARY_PATH environment variable already set (as I happen to do), then that line will not overwrite it with the value needed for the test.

[wallace]

Thanks!

[jingham]

This is fine. If there were a universal convention (e.g. : separated lists) for environment variables, it would be reasonable for "append" behavior to extend rather than replace extant variables, but in the absence of the universality of such a convention, the best we can do is overwrite.

[davide]

This broke macOS:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/22425/

I reverted it in:

commit 27d52cd86a2cf82214b71519dffd450c54cf87ae (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <[e-mail]>
Date:   Wed Jul 8 13:00:29 2020 -0700
    Revert "[lldb/API] Overwrite variables with SBLaunchInfo::SetEnvironment(append=true)"
    This reverts commit 695b33a56919af8873eecb47cb83fa17a271e99f beacuse
    it broke the macOS bot.

Let me know if you need any help/info to reproduce [just shoot me an e-mail], but ToT + ninja check-lldb on a recent'ish 10.15 should do it.

[labath]

Thanks. It was fairly easy to reproduce the problem. I have a proposed fix in D83552.

lldb/source/API/SBLaunchInfo.cpp

 //===-- SBLaunchInfo.cpp --------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/API/SBLaunchInfo.h"
 #include "SBReproducerPrivate.h"
 
 #include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBListener.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 
 using namespace lldb;
LLDB_RECORD_METHOD(void, SBLaunchInfo, SetEnvironmentEntries,
                      (const char **, bool), envp, append);
   SetEnvironment(SBEnvironment(Environment(envp)), append);
 }
 
 void SBLaunchInfo::SetEnvironment(const SBEnvironment &env, bool append) {
   LLDB_RECORD_METHOD(void, SBLaunchInfo, SetEnvironment,
                      (const lldb::SBEnvironment &, bool), env, append);
   Environment &refEnv = env.ref();
-  if (append)
-    m_opaque_sp->GetEnvironment().insert(refEnv.begin(), refEnv.end());
-  else
+  if (append) {
+    for (auto &KV : refEnv)
+      m_opaque_sp->GetEnvironment().insert_or_assign(KV.first(), KV.second);
+  } else
     m_opaque_sp->GetEnvironment() = refEnv;
   m_opaque_sp->RegenerateEnvp();
 }
 
 SBEnvironment SBLaunchInfo::GetEnvironment() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBLaunchInfo, GetEnvironment);
   return LLDB_RECORD_RESULT(
       SBEnvironment(Environment(m_opaque_sp->GetEnvironment())));

lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

def test_launch_info(self):
         self.assertEqual(env.GetNumValues(), env_count + 1)
 
         # Make sure we only modify the copy of the launchInfo's environment
         self.assertEqual(launch_info.GetEnvironment().GetNumValues(), env_count)
 
         launch_info.SetEnvironment(env, append=True)
         self.assertEqual(launch_info.GetEnvironment().GetNumValues(), env_count + 1)
 
+        env.Set("FOO", "baz", overwrite=True)
+        launch_info.SetEnvironment(env, append=True)
+        self.assertEqual(launch_info.GetEnvironment().GetNumValues(), env_count + 1)
+        self.assertEqual(launch_info.GetEnvironment().Get("FOO"), "baz")
+
         # Make sure we can replace the launchInfo's environment
         env.Clear()
         env.Set("BAR", "foo", overwrite=True)
         env.PutEntry("X=y")
         launch_info.SetEnvironment(env, append=False)
         self.assertEqualEntries(launch_info.GetEnvironment(), ["BAR=foo", "X=y"])
 
 
def test_creating_and_modifying_environment(self):
         entries.AppendList(["X=x", "Y=y"], 2)
 
         env.SetEntries(entries, append=True)
         self.assertEqualEntries(env, ["BAR=foo", "X=x", "Y=y"])
 
         env.SetEntries(entries, append=False)
         self.assertEqualEntries(env, ["X=x", "Y=y"])
 
+        entries.Clear()
+        entries.AppendList(["X=y", "Y=x"], 2)
+        env.SetEntries(entries, append=True)
+        self.assertEqualEntries(env, ["X=y", "Y=x"])
+
         # Test clear
         env.Clear()
         self.assertEqualEntries(env, [])