Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by jasonmolenda on Aug 17 2023, 5:22 PM.



We have an ASAN+UBSAN bot for lldb with swift supported added on - - 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());
                               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.

Diff Detail

Event Timeline

jasonmolenda created this revision.Aug 17 2023, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 5:22 PM
jasonmolenda requested review of this revision.Aug 17 2023, 5:22 PM
jingham accepted this revision.Aug 17 2023, 5:45 PM
jingham added a subscriber: 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...

This revision is now accepted and ready to land.Aug 17 2023, 5:45 PM