Change LLGSTest.cpp to run environment_check inferior with stdin/stdout disabled, to work around ASAN CI bot issue

[jasonmolenda]

Summary

We have an ASAN+UBSAN bot for lldb with swift supported added on swift.org - https://ci.swift.org/view/LLDB/job/oss-lldb-asan-macos/ - and it has been failing for a few months, it seems. The failing test is the LLDBServerTests. There is a test that is setting an environment variable and running lldb/unittests/tools/lldb-server/inferior/environment_check.cpp to confirm (via the exit value) that the env var was set. The test in lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp launches the app, continues and expects it to exit:

ASSERT_THAT_ERROR(Client.ContinueAll(), Succeeded());
ASSERT_THAT_EXPECTED(
    Client.GetLatestStopReplyAs<StopReplyExit>(),
    HasValue(testing::Property(&StopReply::getKind,
                               WaitStatus{WaitStatus::Exit, 0})));

This ContinueAll ends up in unittests/tools/lldb-server/tests/TestClient.cpp TestClient::Continue() which sends a continue packet, and expects to see either a stop reply packet or a process-exited response,

[...]
  auto StopReplyOr = SendMessage<StopReply>(
      message, m_process_info->GetEndian(), m_register_infos);
  if (!StopReplyOr)
    return StopReplyOr.takeError();

  m_stop_reply = std::move(*StopReplyOr);
  if (!isa<StopReplyStop>(m_stop_reply)) {
    StringExtractorGDBRemote R;
    PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
[...]

When run on the UBSan bot, the inferior (environment_check) is also built ASAN and while it is running, asan or a malloc library prints a note to stderr, environment_check(41292,0x113e7a600) malloc: nano zone abandoned due to inability to preallocate reserved vm space.. When we look at the bot logging, we see the problem:

[  INFO ] /Users/ec2-user/jenkins/workspace/oss-lldb-asan-macos/llvm-project/lldb/unittests/tools/lldb-server/tests/TestClient.cpp:201:: Send Packet: vCont;c

[  INFO ] /Users/ec2-user/jenkins/workspace/oss-lldb-asan-macos/llvm-project/lldb/unittests/tools/lldb-server/tests/TestClient.cpp:204:: Read Packet: O656e7669726f6e6d656e745f636865636b2833323338352c307831313535623336303029206d616c6c6f633a206e616e6f207a6f6e65206162616e646f6e65642064756520746f20696e6162696c69747920746f20707265616c6c6f6361746520726573657276656420766d2073706163652e0d0a
/Users/ec2-user/jenkins/workspace/oss-lldb-asan-macos/llvm-project/lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp:50: Failure
Value of: llvm::detail::TakeError(Client.ContinueAll())
Expected: succeeded
  Actual: failed  (Unable to parse StopReply: Invalid packet)

We sent a vCont;c to continue, and some stdout/stderr text was received while it was running. And the next packet we receive is almost certainly the "process has exited successfully" result, and the check in TestClient::Continue() would have succeeded.

There's no stdin/stdout/stderr needed with environment_check, so the easiest fix I'd like to try is to have it launched with these channels not sent up to lldb at all. We should see the vCont;c followed by the "process exited successfully" packet and everyone will be happy.

This test is originally @labath but I don't want to assume he'll have time to look at this. Let's see if anyone has an opinion, Pavel don't feel obligated to weigh in unless you have time/want to.

[jingham]

This seems good to me. Tests that aren't using stdio for a purpose shouldn't have to worry about random output.

But wait on my acceptance a bit to see if Pavel (or anyone else) has a stronger opinion...

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

ASSERT_THAT_EXPECTED(
                                  WaitStatus{WaitStatus::Exit, 0})));
 }
 #endif
 
 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")});
+  auto ClientOr = TestClient::launchCustom(
+      getLogFileName(), /* disable_stdio */ true,
+      {"--env", "LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE"},
   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/unittests/tools/lldb-server/tests/TestClient.h

public:
   /// When the client connects, it already has a process ready.
   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);
-
+  launchCustom(llvm::StringRef Log, bool disable_stdio,
+               llvm::ArrayRef<llvm::StringRef> ServerArgs,
+               llvm::ArrayRef<llvm::StringRef> InferiorArgs);
 
   ~TestClient() override;
   llvm::Error SetInferior(llvm::ArrayRef<std::string> inferior_args);
   llvm::Error ListThreadsInStopReply();
   llvm::Error SetBreakpoint(unsigned long address);
   llvm::Error ContinueAll();
   llvm::Error ContinueThread(unsigned long thread_id);
   const ProcessInfo &GetProcessInfo();

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

Error TestClient::initializeConnection() {
   return Error::success();
 }
 
 Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log) {
   return launch(Log, {});
 }
 
 Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log, ArrayRef<StringRef> InferiorArgs) {
-  return launchCustom(Log, {}, InferiorArgs);
+  return launchCustom(Log, false, {}, InferiorArgs);
 }
 
-Expected<std::unique_ptr<TestClient>> TestClient::launchCustom(StringRef Log, ArrayRef<StringRef> ServerArgs, ArrayRef<StringRef> InferiorArgs) {
+Expected<std::unique_ptr<TestClient>>
+TestClient::launchCustom(StringRef Log, bool disable_stdio,
+                         ArrayRef<StringRef> ServerArgs,
+                         ArrayRef<StringRef> InferiorArgs) {
   const ArchSpec &arch_spec = HostInfo::GetArchitecture();
   Args args;
   args.AppendArgument(LLDB_SERVER);
   if (IsLldbServer())
     args.AppendArgument("gdbserver");
   args.AppendArgument("--reverse-connect");
 
   if (!Log.empty()) {
TestClient::launchCustom(StringRef Log, bool disable_stdio,
   Info.SetArchitecture(arch_spec);
   Info.SetArguments(args, true);
   Info.GetEnvironment() = Host::GetEnvironment();
   // TODO: Use this callback to detect botched launches. If lldb-server does not
   // start, we can print a nice error message here instead of hanging in
   // Accept().
   Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback);
 
+  if (disable_stdio)
+    Info.GetFlags().Set(lldb::eLaunchFlagDisableSTDIO);
   status = Host::LaunchProcess(Info);
   if (status.Fail())
     return status.ToError();
 
   Socket *accept_socket;
   listen_socket.Accept(accept_socket);
   auto Conn = std::make_unique<ConnectionFileDescriptor>(accept_socket);
   auto Client = std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));