[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL322174: Add Utility/Environment class for handling... 
environments (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D41359?vs=127384=129239#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41359

Files:
  lldb/trunk/include/lldb/API/SBLaunchInfo.h
  lldb/trunk/include/lldb/Host/Host.h
  lldb/trunk/include/lldb/Interpreter/Args.h
  lldb/trunk/include/lldb/Target/Platform.h
  lldb/trunk/include/lldb/Target/ProcessInfo.h
  lldb/trunk/include/lldb/Target/Target.h
  lldb/trunk/include/lldb/Utility/Environment.h
  
lldb/trunk/packages/Python/lldbsuite/test/python_api/sblaunchinfo/TestSBLaunchInfo.py
  lldb/trunk/source/API/SBLaunchInfo.cpp
  lldb/trunk/source/API/SBPlatform.cpp
  lldb/trunk/source/API/SBProcess.cpp
  lldb/trunk/source/API/SBTarget.cpp
  lldb/trunk/source/Commands/CommandObjectProcess.cpp
  lldb/trunk/source/Host/freebsd/Host.cpp
  lldb/trunk/source/Host/linux/Host.cpp
  lldb/trunk/source/Host/macosx/Host.mm
  lldb/trunk/source/Host/netbsd/Host.cpp
  lldb/trunk/source/Host/openbsd/Host.cpp
  lldb/trunk/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/trunk/source/Host/windows/Host.cpp
  lldb/trunk/source/Interpreter/Args.cpp
  lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  
lldb/trunk/source/Plugins/Platform/MacOSX/PlatformiOSSimulatorCoreSimulatorSupport.mm
  lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.h
  lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.h
  lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/trunk/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  lldb/trunk/source/Target/Platform.cpp
  lldb/trunk/source/Target/Process.cpp
  lldb/trunk/source/Target/ProcessInfo.cpp
  lldb/trunk/source/Target/Target.cpp
  lldb/trunk/source/Utility/CMakeLists.txt
  lldb/trunk/source/Utility/Environment.cpp
  lldb/trunk/tools/lldb-server/lldb-gdbserver.cpp
  lldb/trunk/unittests/Host/HostTest.cpp
  lldb/trunk/unittests/Interpreter/TestArgs.cpp
  lldb/trunk/unittests/Utility/CMakeLists.txt
  lldb/trunk/unittests/Utility/EnvironmentTest.cpp
  lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp

Index: lldb/trunk/include/lldb/Target/ProcessInfo.h
===
--- lldb/trunk/include/lldb/Target/ProcessInfo.h
+++ lldb/trunk/include/lldb/Target/ProcessInfo.h
@@ -13,6 +13,7 @@
 // LLDB headers
 #include "lldb/Interpreter/Args.h"
 #include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Environment.h"
 #include "lldb/Utility/FileSpec.h"
 
 namespace lldb_private {
@@ -81,18 +82,17 @@
 
   void SetArguments(char const **argv, bool first_arg_is_executable);
 
-  Args () { return m_environment; }
-
-  const Args () const { return m_environment; }
+  Environment () { return m_environment; }
+  const Environment () const { return m_environment; }
 
 protected:
   FileSpec m_executable;
   std::string m_arg0; // argv[0] if supported. If empty, then use m_executable.
   // Not all process plug-ins support specifying an argv[0]
   // that differs from the resolved platform executable
   // (which is in m_executable)
   Args m_arguments; // All program arguments except argv[0]
-  Args m_environment;
+  Environment m_environment;
   uint32_t m_uid;
   uint32_t m_gid;
   ArchSpec m_arch;
Index: lldb/trunk/include/lldb/Target/Platform.h
===
--- lldb/trunk/include/lldb/Target/Platform.h
+++ lldb/trunk/include/lldb/Target/Platform.h
@@ -633,7 +633,7 @@
   //--
   virtual Status Install(const FileSpec , const FileSpec );
 
-  virtual size_t GetEnvironment(StringList );
+  virtual Environment GetEnvironment();
 
   virtual bool GetFileExists(const lldb_private::FileSpec _spec);
 
Index: lldb/trunk/include/lldb/Target/Target.h
===
--- lldb/trunk/include/lldb/Target/Target.h
+++ lldb/trunk/include/lldb/Target/Target.h
@@ -115,9 +115,8 @@
 
   void SetRunArguments(const Args );
 
-  size_t GetEnvironmentAsArgs(Args ) const;
-
-  void SetEnvironmentFromArgs(const Args );
+  Environment GetEnvironment() const;
+  void SetEnvironment(Environment env);
 
   bool GetSkipPrologue() const;
 
Index: 

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Thanks for the explanation. Good to go.


https://reviews.llvm.org/D41359



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: include/lldb/Target/Platform.h:636
 
-  virtual size_t GetEnvironment(StringList );
+  virtual Environment GetEnvironment();
 

clayborg wrote:
> Do we want to return by value? Not a const reference?
This object can come from the Host::GetEnvironment, which constructs it 
on-demand. Returning a reference would mean we need to cache a value in the 
host layer, but that's tricky as the environment can change during runtime, and 
you want to make sure you always return an up-to-date value.

I don't think this is an issue, as all the returns will be moves, so the 
situation will be the same as before this patch (where we also created a copy 
of the environment in the by-ref argument). 



Comment at: include/lldb/Target/Target.h:118-119
 
-  size_t GetEnvironmentAsArgs(Args ) const;
-
-  void SetEnvironmentFromArgs(const Args );
+  Environment GetEnvironment() const;
+  void SetEnvironment(Environment env);
 

clayborg wrote:
> set/get using reference instead of by value?
There is no persistent object object to back the reference for the getter (it's 
constructed from an OptionValueDictionary). Using a value for the setter is 
actually better, as that allows me to move-assign the environment object in the 
LaunchInfo. If the user calls this with a moved object then we get no copies.



Comment at: source/API/SBLaunchInfo.cpp:34
+private:
+  Environment::Envp m_envp;
+};

clayborg wrote:
> Seems like a lot of work to get indexed environment variable access. Should 
> we not make the Environment class use a std::vector instead of a map? Not 
> sure if environment variable order is ever important? Is so then the 
> StringMap isn't what we want to use. I know we have it in our public API so 
> we can't change it now and thus why we need this work around. Just thinking 
> out loud
I was thinking about the importance of variable order as well. I can certainly 
construct an application that will behave differently depending on the order of 
environment variables it is given. However, something like that seems unlikely 
to happen for real.

I actually started working on an order-preserving implementation, but then 
dropped it when I saw that we are already shoving the variables through a 
std::map in the OptionValueDictionary in target.env-vars setting. If we manage 
to drop that (OptionValueEnvironment?), then I think it would make sense to 
swap out this implementation for an order-preserving one. I would still keep 
the present map-like interface, as we do key-based lookup in quite a lot of 
places.


https://reviews.llvm.org/D41359



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

ok as long as we don't want to return const reference when returning 
Environment values in getters and setters.




Comment at: include/lldb/Target/Platform.h:636
 
-  virtual size_t GetEnvironment(StringList );
+  virtual Environment GetEnvironment();
 

Do we want to return by value? Not a const reference?



Comment at: include/lldb/Target/Target.h:118-119
 
-  size_t GetEnvironmentAsArgs(Args ) const;
-
-  void SetEnvironmentFromArgs(const Args );
+  Environment GetEnvironment() const;
+  void SetEnvironment(Environment env);
 

set/get using reference instead of by value?



Comment at: source/API/SBLaunchInfo.cpp:34
+private:
+  Environment::Envp m_envp;
+};

Seems like a lot of work to get indexed environment variable access. Should we 
not make the Environment class use a std::vector instead of a map? Not sure if 
environment variable order is ever important? Is so then the StringMap isn't 
what we want to use. I know we have it in our public API so we can't change it 
now and thus why we need this work around. Just thinking out loud


https://reviews.llvm.org/D41359



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 127384.
labath added a comment.

re-clang-format the patch


https://reviews.llvm.org/D41359

Files:
  include/lldb/API/SBLaunchInfo.h
  include/lldb/Host/Host.h
  include/lldb/Interpreter/Args.h
  include/lldb/Target/Platform.h
  include/lldb/Target/ProcessInfo.h
  include/lldb/Target/Target.h
  include/lldb/Utility/Environment.h
  packages/Python/lldbsuite/test/python_api/sblaunchinfo/TestSBLaunchInfo.py
  source/API/SBLaunchInfo.cpp
  source/API/SBPlatform.cpp
  source/API/SBProcess.cpp
  source/API/SBTarget.cpp
  source/Commands/CommandObjectProcess.cpp
  source/Host/freebsd/Host.cpp
  source/Host/linux/Host.cpp
  source/Host/macosx/Host.mm
  source/Host/netbsd/Host.cpp
  source/Host/openbsd/Host.cpp
  source/Host/posix/ProcessLauncherPosixFork.cpp
  source/Host/windows/Host.cpp
  source/Interpreter/Args.cpp
  source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  source/Plugins/Platform/MacOSX/PlatformiOSSimulatorCoreSimulatorSupport.mm
  source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  source/Plugins/Platform/POSIX/PlatformPOSIX.h
  source/Plugins/Platform/Windows/PlatformWindows.cpp
  source/Plugins/Platform/Windows/PlatformWindows.h
  source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
  source/Target/Platform.cpp
  source/Target/Process.cpp
  source/Target/ProcessInfo.cpp
  source/Target/Target.cpp
  source/Utility/CMakeLists.txt
  source/Utility/Environment.cpp
  tools/lldb-server/lldb-gdbserver.cpp
  unittests/Host/HostTest.cpp
  unittests/Interpreter/TestArgs.cpp
  unittests/Utility/CMakeLists.txt
  unittests/Utility/EnvironmentTest.cpp
  unittests/tools/lldb-server/tests/TestClient.cpp

Index: unittests/tools/lldb-server/tests/TestClient.cpp
===
--- unittests/tools/lldb-server/tests/TestClient.cpp
+++ unittests/tools/lldb-server/tests/TestClient.cpp
@@ -89,10 +89,7 @@
   ProcessLaunchInfo Info;
   Info.SetArchitecture(arch_spec);
   Info.SetArguments(args, true);
-
-  StringList Env;
-  Host::GetEnvironment(Env);
-  Info.GetEnvironmentEntries() = Args(Env);
+  Info.GetEnvironment() = Host::GetEnvironment();
 
   status = Host::LaunchProcess(Info);
   if (status.Fail())
@@ -112,14 +109,9 @@
 }
 
 Error TestClient::SetInferior(llvm::ArrayRef inferior_args) {
-  StringList env;
-  Host::GetEnvironment(env);
-  for (size_t i = 0; i < env.GetSize(); ++i) {
-if (SendEnvironmentPacket(env[i].c_str()) != 0) {
-  return make_error(
-  formatv("Failed to set environment variable: {0}", env[i]).str(),
-  inconvertibleErrorCode());
-}
+  if (SendEnvironment(Host::GetEnvironment()) != 0) {
+return make_error("Failed to set launch environment",
+   inconvertibleErrorCode());
   }
   std::stringstream command;
   command << "A";
Index: unittests/Utility/EnvironmentTest.cpp
===
--- /dev/null
+++ unittests/Utility/EnvironmentTest.cpp
@@ -0,0 +1,49 @@
+//===-- EnvironmentTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Environment.h"
+
+using namespace lldb_private;
+
+TEST(EnvironmentTest, EnvpConstruction) {
+  const char **Envp1 = nullptr;
+  EXPECT_EQ(0u, Environment(Envp1).size());
+
+  const char *Envp2[] = {"FOO=BAR", nullptr};
+  EXPECT_EQ("BAR", Environment(Envp2).lookup("FOO"));
+
+  const char *Envp3[] = {"FOO=BAR", "FOO=BAZ", nullptr};
+  EXPECT_EQ("BAR", Environment(Envp3).lookup("FOO"));
+
+  const char *Envp4[] = {"FOO=", "BAR", nullptr};
+  Environment Env4(Envp4);
+  ASSERT_EQ(2u, Env4.size());
+  EXPECT_EQ("", Environment(Envp4).find("FOO")->second);
+  EXPECT_EQ("", Environment(Envp4).find("BAR")->second);
+
+  const char *Envp5[] = {"FOO=BAR=BAZ"};
+  EXPECT_EQ("BAR=BAZ", Environment(Envp5).lookup("FOO"));
+}
+
+TEST(EnvironmentTest, EnvpConversion) {
+  std::string FOO_EQ_BAR("FOO=BAR");
+  std::string BAR_EQ_BAZ("BAR=BAZ");
+
+  Environment Env;
+  Env.insert(FOO_EQ_BAR);
+  Env.insert(BAR_EQ_BAZ);
+  Environment::Envp Envp = Env.getEnvp();
+  const char *const *Envp_ = Envp;
+
+  EXPECT_TRUE(FOO_EQ_BAR == Envp_[0] || FOO_EQ_BAR == Envp_[1]);
+  EXPECT_TRUE(BAR_EQ_BAZ == Envp_[0] || BAR_EQ_BAZ == Envp_[1]);
+  EXPECT_EQ(nullptr, 

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.



Comment at: include/lldb/Utility/Environment.h:70-72
+  std::pair insert(llvm::StringRef KeyEqValue) {
+return insert(KeyEqValue.split('='));
+  }

zturner wrote:
> Why'd you decide to go with inserting an entire pre-formatted key value pair 
> in the form `{key}={value}`, instead of having the function be 
> `insert(StringRef Key, StringRef Value)`?
> 
The `insert(key, value)` form is still available (and used where appropriate). 
This is a convenience function because in a lot of the cases we only have the 
"KEY=VALUE" form handy (constructing from `char**envp`, reading the env over 
gdb-remote, ...), and I wanted to avoid having each user having to split this  
manually. I wouldn't be opposed to calling this something other than `insert` 
if you think it's confusing, but I think the functionality per-se is quite 
useful.


https://reviews.llvm.org/D41359



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: include/lldb/Utility/Environment.h:70-72
+  std::pair insert(llvm::StringRef KeyEqValue) {
+return insert(KeyEqValue.split('='));
+  }

Why'd you decide to go with inserting an entire pre-formatted key value pair in 
the form `{key}={value}`, instead of having the function be `insert(StringRef 
Key, StringRef Value)`?




Comment at: source/Host/macosx/Host.mm:763
 
-proc_env.AppendArgument(llvm::StringRef(cstr));
+   proc_env.insert(cstr);
   }

Indentation is messed up here.


https://reviews.llvm.org/D41359



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits