llgs: Propagate the environment when launching the inferior from command line

[labath]

Summary

We were failing to propagate the environment when lldb-server was
started with a pre-loaded process
(e.g.: lldb-server gdbserver -- inferior --inferior_args)

This patch makes sure the environment is propagated. Instead of adding a
new GDBRemoteCommunicationServerLLGS::SetLaunchEnvironment function to
complement SetLaunchArgs and SetLaunchFlags, I replace these with a
more generic SetLaunchInfo, which can be used to set any launch-related
property.

The accompanying test also verifies that the server correctly terminates
the connection after sending the exit packet (specifically, that it does
not send the exit packet twice).

[clayborg]

Anything that launches a process should use the ProcessLaunchInfo. Nice patch.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

46 ↗

(On Diff #126367)

We might want a "ProcessLaunchInfo &" accessor so we can modify the flags after they have initially been set?

ProcessLaunchInfo &GetLaunchInfo() { return m_process_launch_info; }

If we don't need it, we shouldn't add it yet, just thinking out loud here.

tools/lldb-server/lldb-gdbserver.cpp

181–182 ↗

(On Diff #126367)

How many places do we set these flags like this? Wondering if we should add a method like:

info.SetFlagsForDebugging();

That way if we add new flags that must be set later, it will be easier than updating all sites that modify these flags for debugging a process.

[labath]

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

46 ↗

(On Diff #126367)

We don't need that at the moment. The launching process is a fire-and-forget kind of thing right now.

tools/lldb-server/lldb-gdbserver.cpp

181–182 ↗

(On Diff #126367)

I've found two places that blindly set the launch flags: here and SBLaunchInfo (the other launches do the dance of consulting the target property for the value). I can make a eLaunchFlagStandardDebug which would include Debug and DisableASLR flags (since we can assume that one normally wants to disable aslr when debugging). However, the StopAtEntry flag could not be there, as it's appropriateness depends on the environment the launch is happening in (here we want it because we need to wait for the client to connect, but this is not appropriate as the SBLaunchInfo default).

[clayborg]

No worries then. No need to make a new enum if this is just two places and they aren't all setting the same flags.

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

 public:
   //------------------------------------------------------------------
   // Constructors and Destructors
   //------------------------------------------------------------------
   GDBRemoteCommunicationServerLLGS(
       MainLoop &mainloop,
       const NativeProcessProtocol::Factory &process_factory);
 
-  //------------------------------------------------------------------
-  /// Specify the program to launch and its arguments.
-  ///
-  /// @param[in] args
-  ///     The command line to launch.
-  ///
-  /// @param[in] argc
-  ///     The number of elements in the args array of cstring pointers.
-  ///
-  /// @return
-  ///     An Status object indicating the success or failure of making
-  ///     the setting.
-  //------------------------------------------------------------------
-  Status SetLaunchArguments(const char *const args[], int argc);
-
-  //------------------------------------------------------------------
-  /// Specify the launch flags for the process.
-  ///
-  /// @param[in] launch_flags
-  ///     The launch flags to use when launching this process.
-  ///
-  /// @return
-  ///     An Status object indicating the success or failure of making
-  ///     the setting.
-  //------------------------------------------------------------------
-  Status SetLaunchFlags(unsigned int launch_flags);
+  void SetLaunchInfo(const ProcessLaunchInfo &info);
 
   //------------------------------------------------------------------
   /// Launch a process with the current launch settings.
   ///
   /// This method supports running an lldb-gdbserver or similar
   /// server in a situation where the startup code has been provided
   /// with all the information for a child process to be launched.
   ///

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

void GDBRemoteCommunicationServerLLGS::RegisterPacketHandlers() {
   RegisterPacketHandler(StringExtractorGDBRemote::eServerPacketType_k,
                         [this](StringExtractorGDBRemote packet, Status &error,
                                bool &interrupt, bool &quit) {
                           quit = true;
                           return this->Handle_k(packet);
                         });
 }
 
-Status
-GDBRemoteCommunicationServerLLGS::SetLaunchArguments(const char *const args[],
-                                                     int argc) {
-  if ((argc < 1) || !args || !args[0] || !args[0][0])
-    return Status("%s: no process command line specified to launch",
-                  __FUNCTION__);
-
-  m_process_launch_info.SetArguments(const_cast<const char **>(args), true);
-  return Status();
-}
-
-Status
-GDBRemoteCommunicationServerLLGS::SetLaunchFlags(unsigned int launch_flags) {
-  m_process_launch_info.GetFlags().Set(launch_flags);
-  return Status();
+void GDBRemoteCommunicationServerLLGS::SetLaunchInfo(const ProcessLaunchInfo &info) {
+  m_process_launch_info = info;
 }
 
 Status GDBRemoteCommunicationServerLLGS::LaunchProcess() {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
   if (!m_process_launch_info.GetArguments().GetArgumentCount())
     return Status("%s: no process command line specified to launch",
                   __FUNCTION__);

lldb/trunk/tools/lldb-server/lldb-gdbserver.cpp

if (end_p &&
           attach_target.size())
     handle_attach_to_pid(gdb_server, static_cast<lldb::pid_t>(pid));
   else
     handle_attach_to_process_name(gdb_server, attach_target);
 }
 
 void handle_launch(GDBRemoteCommunicationServerLLGS &gdb_server, int argc,
                    const char *const argv[]) {
-  Status error;
-  error = gdb_server.SetLaunchArguments(argv, argc);
-  if (error.Fail()) {
-    fprintf(stderr, "error: failed to set launch args for '%s': %s\n", argv[0],
-            error.AsCString());
+  ProcessLaunchInfo info;
+  info.GetFlags().Set(eLaunchFlagStopAtEntry | eLaunchFlagDebug |
+                      eLaunchFlagDisableASLR);
+  info.SetArguments(const_cast<const char **>(argv), true);
+
+  llvm::SmallString<64> cwd;
+  if (std::error_code ec = llvm::sys::fs::current_path(cwd)) {
+    llvm::errs() << "Error getting current directory: " << ec.message() << "\n";
     exit(1);
   }
+  info.SetWorkingDirectory(FileSpec(cwd, true));
 
-  unsigned int launch_flags = eLaunchFlagStopAtEntry | eLaunchFlagDebug;
+  StringList env;
+  Host::GetEnvironment(env);
+  info.GetEnvironmentEntries() = Args(env);
 
-  error = gdb_server.SetLaunchFlags(launch_flags);
-  if (error.Fail()) {
-    fprintf(stderr, "error: failed to set launch flags for '%s': %s\n", argv[0],
-            error.AsCString());
-    exit(1);
-  }
+  gdb_server.SetLaunchInfo(info);
 
-  error = gdb_server.LaunchProcess();
+  Status error = gdb_server.LaunchProcess();
   if (error.Fail()) {
-    fprintf(stderr, "error: failed to launch '%s': %s\n", argv[0],
-            error.AsCString());
+    llvm::errs() << llvm::formatv("error: failed to launch '{0}': {1}\n",
+                                  argv[0], error);
     exit(1);
   }
 }
 
 Status writeSocketIdToPipe(Pipe &port_pipe, const std::string &socket_id) {
   size_t bytes_written = 0;
   // Write the port number as a C string with the NULL terminator.
   return port_pipe.Write(socket_id.c_str(), socket_id.size() + 1,

lldb/trunk/unittests/tools/lldb-server/CMakeLists.txt

 set(ALL_LLDB_TEST_EXECUTABLES)
 
 function(add_lldb_test_executable test_name)
   set(EXCLUDE_FROM_ALL ON)
   add_llvm_executable(${test_name} NO_INSTALL_RPATH ${ARGN})
   set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
   set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
   list(APPEND ALL_LLDB_TEST_EXECUTABLES ${test_name})
   set(ALL_LLDB_TEST_EXECUTABLES ${ALL_LLDB_TEST_EXECUTABLES} PARENT_SCOPE)
 endfunction()
 
 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>")
 else()
   add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>")
 endif()
 
 add_definitions(
   -DLLDB_TEST_INFERIOR_PATH="${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}"
   -DLLDB_TEST_INFERIOR_SUFFIX="${CMAKE_EXECUTABLE_SUFFIX}"
   )
 add_subdirectory(tests)

lldb/trunk/unittests/tools/lldb-server/inferior/environment_check.cpp

+//===-- thread_inferior.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <string>
+#include <cstdlib>
+
+int main() {
+  const char *value = std::getenv("LLDB_TEST_MAGIC_VARIABLE");
+  if (!value)
+    return 1;
+  if (std::string(value) != "LLDB_TEST_MAGIC_VALUE")
+    return 2;
+  return 0;
+}

lldb/trunk/unittests/tools/lldb-server/tests/CMakeLists.txt

 add_lldb_unittest(LLDBServerTests
+  LLGSTest.cpp
+  MessageObjects.cpp
   TestBase.cpp
   TestClient.cpp
-  MessageObjects.cpp
   ThreadIdsInJstopinfoTest.cpp
 
   LINK_LIBS
     lldbHost
     lldbCore
     lldbInterpreter
     lldbTarget
     lldbPluginPlatformLinux
     lldbPluginProcessGDBRemote
 
     LLVMTestingSupport
   LINK_COMPONENTS
     Support
   )
 
 add_dependencies(LLDBServerTests lldb-server ${ALL_LLDB_TEST_EXECUTABLES})

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

+//===-- LLGSTest.cpp --------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestBase.h"
+#include "lldb/Host/Host.h"
+#include "llvm/Testing/Support/Error.h"
+
+using namespace llgs_tests;
+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(),
+                                     {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})));
+}

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

if (!InferiorArgs.empty()) {
     args.AppendArgument("--");
     for (StringRef arg : InferiorArgs)
       args.AppendArgument(arg);
   }
 
   ProcessLaunchInfo Info;
   Info.SetArchitecture(arch_spec);
   Info.SetArguments(args, true);
+
+  StringList Env;
+  Host::GetEnvironment(Env);
+  Info.GetEnvironmentEntries() = Args(Env);
+
   status = Host::LaunchProcess(Info);
   if (status.Fail())
     return status.ToError();
 
   Socket *accept_socket;
   listen_socket.Accept(accept_socket);
   auto Conn = llvm::make_unique<ConnectionFileDescriptor>(accept_socket);
-  return std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));
+  auto Client = std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));
+
+  if (!InferiorArgs.empty()) {
+    if (Error E = Client->QueryProcessInfo())
+      return std::move(E);
+  }
+
+  return std::move(Client);
 }
 
 Error TestClient::SetInferior(llvm::ArrayRef<std::string> 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<StringError>(