Skip to content

Commit d8b3c1a

Browse files
committedDec 18, 2017
NPL: Clean up handling of inferior exit
Summary: lldb-server was sending the "exit" packet (W??) twice. This happened because it was handling both the pre-exit (PTRACE_EVENT_EXIT) and post-exit (WIFEXITED) as exit events. We had some code which was trying to detect when we've already sent the exit packet, but this stopped working quite a while ago. This never really caused any problems in practice because the client automatically closes the connection after receiving the first packet, so the only effect of this was some warning messages about extra packets from the lldb-server test suite, which were ignored because they didn't fail the test. The new test suite will be stricter about this, so I fix this issue ignoring the first event. I think this is the correct behavior, as the inferior is not really dead at that point, so it's premature to send the exit packet. There isn't an actual test yet which would verify the exit behavior, but in my next patch I will add a test which will also test this functionality. Reviewers: eugene Subscribers: lldb-commits Differential Revision: https://reviews.llvm.org/D41069 llvm-svn: 320961
1 parent 6f42de3 commit d8b3c1a

File tree

3 files changed

+31
-47
lines changed

3 files changed

+31
-47
lines changed
 

‎lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

+15-47
Original file line numberDiff line numberDiff line change
@@ -412,46 +412,20 @@ void NativeProcessLinux::MonitorCallback(lldb::pid_t pid, bool exited,
412412

413413
// Handle when the thread exits.
414414
if (exited) {
415-
LLDB_LOG(log, "got exit signal({0}) , tid = {1} ({2} main thread)", signal,
416-
pid, is_main_thread ? "is" : "is not");
415+
LLDB_LOG(log,
416+
"got exit signal({0}) , tid = {1} ({2} main thread), process "
417+
"state = {3}",
418+
signal, pid, is_main_thread ? "is" : "is not", GetState());
417419

418420
// This is a thread that exited. Ensure we're not tracking it anymore.
419-
const bool thread_found = StopTrackingThread(pid);
421+
StopTrackingThread(pid);
420422

421423
if (is_main_thread) {
422-
// We only set the exit status and notify the delegate if we haven't
423-
// already set the process
424-
// state to an exited state. We normally should have received a SIGTRAP |
425-
// (PTRACE_EVENT_EXIT << 8)
426-
// for the main thread.
427-
const bool already_notified = (GetState() == StateType::eStateExited) ||
428-
(GetState() == StateType::eStateCrashed);
429-
if (!already_notified) {
430-
LLDB_LOG(
431-
log,
432-
"tid = {0} handling main thread exit ({1}), expected exit state "
433-
"already set but state was {2} instead, setting exit state now",
434-
pid,
435-
thread_found ? "stopped tracking thread metadata"
436-
: "thread metadata not found",
437-
GetState());
438-
// The main thread exited. We're done monitoring. Report to delegate.
439-
SetExitStatus(status, true);
424+
// The main thread exited. We're done monitoring. Report to delegate.
425+
SetExitStatus(status, true);
440426

441-
// Notify delegate that our process has exited.
442-
SetState(StateType::eStateExited, true);
443-
} else
444-
LLDB_LOG(log, "tid = {0} main thread now exited (%s)", pid,
445-
thread_found ? "stopped tracking thread metadata"
446-
: "thread metadata not found");
447-
} else {
448-
// Do we want to report to the delegate in this case? I think not. If
449-
// this was an orderly thread exit, we would already have received the
450-
// SIGTRAP | (PTRACE_EVENT_EXIT << 8) signal, and we would have done an
451-
// all-stop then.
452-
LLDB_LOG(log, "tid = {0} handling non-main thread exit (%s)", pid,
453-
thread_found ? "stopped tracking thread metadata"
454-
: "thread metadata not found");
427+
// Notify delegate that our process has exited.
428+
SetState(StateType::eStateExited, true);
455429
}
456430
return;
457431
}
@@ -662,10 +636,8 @@ void NativeProcessLinux::MonitorSIGTRAP(const siginfo_t &info,
662636
case (SIGTRAP | (PTRACE_EVENT_EXIT << 8)): {
663637
// The inferior process or one of its threads is about to exit.
664638
// We don't want to do anything with the thread so we just resume it. In
665-
// case we
666-
// want to implement "break on thread exit" functionality, we would need to
667-
// stop
668-
// here.
639+
// case we want to implement "break on thread exit" functionality, we would
640+
// need to stop here.
669641

670642
unsigned long data = 0;
671643
if (GetEventMessage(thread.GetID(), &data).Fail())
@@ -677,18 +649,14 @@ void NativeProcessLinux::MonitorSIGTRAP(const siginfo_t &info,
677649
data, WIFEXITED(data), WIFSIGNALED(data), thread.GetID(),
678650
is_main_thread);
679651

680-
if (is_main_thread)
681-
SetExitStatus(WaitStatus::Decode(data), true);
682652

683653
StateType state = thread.GetState();
684654
if (!StateIsRunningState(state)) {
685655
// Due to a kernel bug, we may sometimes get this stop after the inferior
686-
// gets a
687-
// SIGKILL. This confuses our state tracking logic in ResumeThread(),
688-
// since normally,
689-
// we should not be receiving any ptrace events while the inferior is
690-
// stopped. This
691-
// makes sure that the inferior is resumed and exits normally.
656+
// gets a SIGKILL. This confuses our state tracking logic in
657+
// ResumeThread(), since normally, we should not be receiving any ptrace
658+
// events while the inferior is stopped. This makes sure that the inferior
659+
// is resumed and exits normally.
692660
state = eStateRunning;
693661
}
694662
ResumeThread(thread, state, LLDB_INVALID_SIGNAL_NUMBER);

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

+1
Original file line numberDiff line numberDiff line change
@@ -825,6 +825,7 @@ void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Exited(
825825

826826
// We are ready to exit the debug monitor.
827827
m_exit_now = true;
828+
m_mainloop.RequestTermination();
828829
}
829830

830831
void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(

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

+15
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ TestClient::TestClient(std::unique_ptr<Connection> Conn) {
4040
}
4141

4242
TestClient::~TestClient() {
43+
if (!IsConnected())
44+
return;
45+
4346
std::string response;
4447
// Debugserver (non-conformingly?) sends a reply to the k packet instead of
4548
// simply closing the connection.
@@ -242,6 +245,18 @@ Error TestClient::Continue(StringRef message) {
242245
return E;
243246

244247
m_stop_reply = std::move(*creation);
248+
if (!isa<StopReplyStop>(m_stop_reply)) {
249+
StringExtractorGDBRemote R;
250+
PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
251+
if (result != PacketResult::ErrorDisconnected) {
252+
return make_error<StringError>(
253+
formatv("Expected connection close after receiving {0}. Got {1}/{2} "
254+
"instead.",
255+
response, result, R.GetStringRef())
256+
.str(),
257+
inconvertibleErrorCode());
258+
}
259+
}
245260
return Error::success();
246261
}
247262

0 commit comments

Comments
 (0)
Please sign in to comment.