debugserver: Propagate environment in launch-mode (pr35671)

Summary:
Make sure we propagate environment when starting debugserver with a pre-loaded
inferior. AFAIK, RNBRunLoopLaunchInferior is only called in pre-loaded inferior
scenario, so we can just pick up the debugserver environment instead of trying
to construct an envp from the (empty) context.

This makes debugserver pass an test added for an equivalent lldb-server fix.

Reviewers: jasonmolenda, clayborg

Subscribers: JDevlieghere, lldb-commits

Differential Revision: https://reviews.llvm.org/D41352

git-svn-id: https://llvm.org/svn/llvm-project/lldb/trunk@321355 91177308-0d34-0410-b5e6-96231b3b80d8

Author: labath

tools/debugserver/source/RNBContext.cpp

@@ -42,6 +42,25 @@ const char *RNBContext::EnvironmentAtIndex(size_t index) {
     return NULL;
 }
 
+static std::string GetEnvironmentKey(const std::string &env) {
+  std::string key = env.substr(0, env.find('='));
+  if (!key.empty() && key.back() == '=')
+    key.pop_back();
+  return key;
+}
+
+void RNBContext::PushEnvironmentIfNeeded(const char *arg) {
+  if (!arg)
+    return;
+  std::string arg_key = GetEnvironmentKey(arg);
+
+  for (const std::string &entry: m_env_vec) {
+    if (arg_key == GetEnvironmentKey(entry))
+      return;
+  }
+  m_env_vec.push_back(arg);
+}
+
 const char *RNBContext::ArgumentAtIndex(size_t index) {
   if (index < m_arg_vec.size())
     return m_arg_vec[index].c_str();

tools/debugserver/source/RNBContext.h

@@ -86,6 +86,7 @@ class RNBContext {
     if (arg)
       m_env_vec.push_back(arg);
   }
+  void PushEnvironmentIfNeeded(const char *arg);
   void ClearEnvironment() {
     m_env_vec.erase(m_env_vec.begin(), m_env_vec.end());
   }

tools/debugserver/source/debugserver.cpp

@@ -1020,6 +1020,7 @@ int main(int argc, char *argv[]) {
   optind = 1;
 #endif
 
+  bool forward_env = false;
   while ((ch = getopt_long_only(argc, argv, short_options, g_long_options,
                                 &long_option_index)) != -1) {
     DNBLogDebug("option: ch == %c (0x%2.2x) --%s%c%s\n", ch, (uint8_t)ch,
@@ -1251,14 +1252,7 @@ int main(int argc, char *argv[]) {
       break;
 
     case 'F':
-      // Pass the current environment down to the process that gets launched
-      {
-        char **host_env = *_NSGetEnviron();
-        char *env_entry;
-        size_t i;
-        for (i = 0; (env_entry = host_env[i]) != NULL; ++i)
-          remote->Context().PushEnvironment(env_entry);
-      }
+      forward_env = true;
       break;
 
     case '2':
@@ -1420,6 +1414,18 @@ int main(int argc, char *argv[]) {
   if (start_mode == eRNBRunLoopModeExit)
     return -1;
 
+  if (forward_env || start_mode == eRNBRunLoopModeInferiorLaunching) {
+    // Pass the current environment down to the process that gets launched
+    // This happens automatically in the "launching" mode. For the rest, we
+    // only do that if the user explicitly requested this via --forward-env
+    // argument.
+    char **host_env = *_NSGetEnviron();
+    char *env_entry;
+    size_t i;
+    for (i = 0; (env_entry = host_env[i]) != NULL; ++i)
+      remote->Context().PushEnvironmentIfNeeded(env_entry);
+  }
+
   RNBRunLoopMode mode = start_mode;
   char err_str[1024] = {'\0'};
 

unittests/tools/lldb-server/CMakeLists.txt

@@ -13,9 +13,9 @@ add_lldb_test_executable(thread_inferior inferior/thread_inferior.cpp)
 add_lldb_test_executable(environment_check inferior/environment_check.cpp)
 
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
-  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:debugserver>")
+  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:debugserver>" -DLLDB_SERVER_IS_DEBUGSERVER=1)
 else()
-  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>")
+  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>" -DLLDB_SERVER_IS_DEBUGSERVER=0)
 endif()
 
 add_definitions(

unittests/tools/lldb-server/tests/LLGSTest.cpp

@@ -16,11 +16,6 @@ using namespace lldb_private;
 using namespace llvm;
 
 TEST_F(TestBase, LaunchModePreservesEnvironment) {
-  if (TestClient::IsDebugServer()) {
-    GTEST_LOG_(WARNING) << "Test fails with debugserver: llvm.org/pr35671";
-    return;
-  }
-
   putenv(const_cast<char *>("LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE"));
 
   auto ClientOr = TestClient::launch(getLogFileName(),
@@ -34,3 +29,20 @@ TEST_F(TestBase, LaunchModePreservesEnvironment) {
       HasValue(testing::Property(&StopReply::getKind,
                                  WaitStatus{WaitStatus::Exit, 0})));
 }
+
+TEST_F(TestBase, DS_TEST(DebugserverEnv)) {
+  // Test that --env takes precedence over inherited environment variables.
+  putenv(const_cast<char *>("LLDB_TEST_MAGIC_VARIABLE=foobar"));
+
+  auto ClientOr = TestClient::launchCustom(getLogFileName(),
+      { "--env", "LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE" },
+                                     {getInferiorPath("environment_check")});
+  ASSERT_THAT_EXPECTED(ClientOr, Succeeded());
+  auto &Client = **ClientOr;
+
+  ASSERT_THAT_ERROR(Client.ContinueAll(), Succeeded());
+  ASSERT_THAT_EXPECTED(
+      Client.GetLatestStopReplyAs<StopReplyExit>(),
+      HasValue(testing::Property(&StopReply::getKind,
+                                 WaitStatus{WaitStatus::Exit, 0})));
+}

unittests/tools/lldb-server/tests/TestClient.cpp

@@ -27,11 +27,6 @@ using namespace lldb_private;
 using namespace llvm;
 
 namespace llgs_tests {
-bool TestClient::IsDebugServer() {
-  return sys::path::filename(LLDB_SERVER).contains("debugserver");
-}
-
-bool TestClient::IsLldbServer() { return !IsDebugServer(); }
 
 TestClient::TestClient(std::unique_ptr<Connection> Conn) {
   SetConnection(Conn.release());
@@ -56,6 +51,10 @@ Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log) {
 }
 
 Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log, ArrayRef<StringRef> InferiorArgs) {
+  return launchCustom(Log, {}, InferiorArgs);
+}
+
+Expected<std::unique_ptr<TestClient>> TestClient::launchCustom(StringRef Log, ArrayRef<StringRef> ServerArgs, ArrayRef<StringRef> InferiorArgs) {
   const ArchSpec &arch_spec = HostInfo::GetArchitecture();
   Args args;
   args.AppendArgument(LLDB_SERVER);
@@ -80,6 +79,9 @@ Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log, ArrayRef
   args.AppendArgument(
       ("localhost:" + Twine(listen_socket.GetLocalPortNumber())).str());
 
+  for (StringRef arg : ServerArgs)
+    args.AppendArgument(arg);
+
   if (!InferiorArgs.empty()) {
     args.AppendArgument("--");
     for (StringRef arg : InferiorArgs)

unittests/tools/lldb-server/tests/TestClient.h

@@ -21,12 +21,20 @@
 #include <memory>
 #include <string>
 
+#if LLDB_SERVER_IS_DEBUGSERVER
+#define LLGS_TEST(x) DISABLED_ ## x
+#define DS_TEST(x) x
+#else
+#define LLGS_TEST(x) x
+#define DS_TEST(x) DISABLED_ ## x
+#endif
+
 namespace llgs_tests {
 class TestClient
     : public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
 public:
-  static bool IsDebugServer();
-  static bool IsLldbServer();
+  static bool IsDebugServer() { return LLDB_SERVER_IS_DEBUGSERVER; }
+  static bool IsLldbServer() { return !IsDebugServer(); }
 
   /// Launches the server, connects it to the client and returns the client. If
   /// Log is non-empty, the server will write it's log to this file.
@@ -37,6 +45,13 @@ class TestClient
   static llvm::Expected<std::unique_ptr<TestClient>>
   launch(llvm::StringRef Log, llvm::ArrayRef<llvm::StringRef> InferiorArgs);
 
+  /// Allows user to pass additional arguments to the server. Be careful when
+  /// using this for generic tests, as the two stubs have different
+  /// command-line interfaces.
+  static llvm::Expected<std::unique_ptr<TestClient>>
+  launchCustom(llvm::StringRef Log, llvm::ArrayRef<llvm::StringRef> ServerArgs, llvm::ArrayRef<llvm::StringRef> InferiorArgs);
+
+
   ~TestClient() override;
   llvm::Error SetInferior(llvm::ArrayRef<std::string> inferior_args);
   llvm::Error ListThreadsInStopReply();