jasonmolenda created this revision. jasonmolenda added a reviewer: labath. jasonmolenda added a project: LLDB. Herald added a subscriber: JDevlieghere. Herald added a project: All. jasonmolenda requested review of this revision. Herald added a subscriber: lldb-commits.
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. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D158237 Files: lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp lldb/unittests/tools/lldb-server/tests/TestClient.cpp lldb/unittests/tools/lldb-server/tests/TestClient.h Index: lldb/unittests/tools/lldb-server/tests/TestClient.h =================================================================== --- lldb/unittests/tools/lldb-server/tests/TestClient.h +++ lldb/unittests/tools/lldb-server/tests/TestClient.h @@ -48,8 +48,9 @@ /// 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); Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp =================================================================== --- lldb/unittests/tools/lldb-server/tests/TestClient.cpp +++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp @@ -59,10 +59,13 @@ } 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); @@ -111,6 +114,8 @@ // Accept(). Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback); + if (disable_stdio) + Info.GetFlags().Set(lldb::eLaunchFlagDisableSTDIO); status = Host::LaunchProcess(Info); if (status.Fail()) return status.ToError(); Index: lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp =================================================================== --- lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp +++ lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp @@ -41,9 +41,10 @@ // 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"}, + {getInferiorPath("environment_check")}); ASSERT_THAT_EXPECTED(ClientOr, Succeeded()); auto &Client = **ClientOr;
Index: lldb/unittests/tools/lldb-server/tests/TestClient.h =================================================================== --- lldb/unittests/tools/lldb-server/tests/TestClient.h +++ lldb/unittests/tools/lldb-server/tests/TestClient.h @@ -48,8 +48,9 @@ /// 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); Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp =================================================================== --- lldb/unittests/tools/lldb-server/tests/TestClient.cpp +++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp @@ -59,10 +59,13 @@ } 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); @@ -111,6 +114,8 @@ // Accept(). Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback); + if (disable_stdio) + Info.GetFlags().Set(lldb::eLaunchFlagDisableSTDIO); status = Host::LaunchProcess(Info); if (status.Fail()) return status.ToError(); Index: lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp =================================================================== --- lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp +++ lldb/unittests/tools/lldb-server/tests/LLGSTest.cpp @@ -41,9 +41,10 @@ // 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"}, + {getInferiorPath("environment_check")}); ASSERT_THAT_EXPECTED(ClientOr, Succeeded()); auto &Client = **ClientOr;
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits