[Lldb-commits] [PATCH] D41069: NPL: Clean up handling of inferior exit
This revision was automatically updated to reflect the committed changes.
Closed by commit rL320961: NPL: Clean up handling of inferior exit (authored by
labath, committed by ).
Repository:
rL LLVM
https://reviews.llvm.org/D41069
Files:
lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
Index: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -40,6 +40,9 @@
}
TestClient::~TestClient() {
+ if (!IsConnected())
+return;
+
std::string response;
// Debugserver (non-conformingly?) sends a reply to the k packet instead of
// simply closing the connection.
@@ -242,6 +245,18 @@
return E;
m_stop_reply = std::move(*creation);
+ if (!isa(m_stop_reply)) {
+StringExtractorGDBRemote R;
+PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
+if (result != PacketResult::ErrorDisconnected) {
+ return make_error(
+ formatv("Expected connection close after receiving {0}. Got {1}/{2} "
+ "instead.",
+ response, result, R.GetStringRef())
+ .str(),
+ inconvertibleErrorCode());
+}
+ }
return Error::success();
}
Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -825,6 +825,7 @@
// We are ready to exit the debug monitor.
m_exit_now = true;
+ m_mainloop.RequestTermination();
}
void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(
Index: lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -412,46 +412,20 @@
// Handle when the thread exits.
if (exited) {
-LLDB_LOG(log, "got exit signal({0}) , tid = {1} ({2} main thread)", signal,
- pid, is_main_thread ? "is" : "is not");
+LLDB_LOG(log,
+ "got exit signal({0}) , tid = {1} ({2} main thread), process "
+ "state = {3}",
+ signal, pid, is_main_thread ? "is" : "is not", GetState());
// This is a thread that exited. Ensure we're not tracking it anymore.
-const bool thread_found = StopTrackingThread(pid);
+StopTrackingThread(pid);
if (is_main_thread) {
- // We only set the exit status and notify the delegate if we haven't
- // already set the process
- // state to an exited state. We normally should have received a SIGTRAP |
- // (PTRACE_EVENT_EXIT << 8)
- // for the main thread.
- const bool already_notified = (GetState() == StateType::eStateExited) ||
-(GetState() == StateType::eStateCrashed);
- if (!already_notified) {
-LLDB_LOG(
-log,
-"tid = {0} handling main thread exit ({1}), expected exit state "
-"already set but state was {2} instead, setting exit state now",
-pid,
-thread_found ? "stopped tracking thread metadata"
- : "thread metadata not found",
-GetState());
-// The main thread exited. We're done monitoring. Report to delegate.
-SetExitStatus(status, true);
+ // The main thread exited. We're done monitoring. Report to delegate.
+ SetExitStatus(status, true);
-// Notify delegate that our process has exited.
-SetState(StateType::eStateExited, true);
- } else
-LLDB_LOG(log, "tid = {0} main thread now exited (%s)", pid,
- thread_found ? "stopped tracking thread metadata"
- : "thread metadata not found");
-} else {
- // Do we want to report to the delegate in this case? I think not. If
- // this was an orderly thread exit, we would already have received the
- // SIGTRAP | (PTRACE_EVENT_EXIT << 8) signal, and we would have done an
- // all-stop then.
- LLDB_LOG(log, "tid = {0} handling non-main thread exit (%s)", pid,
- thread_found ? "stopped tracking thread metadata"
-: "thread metadata not found");
+ // Notify delegate that our process has exited.
+ SetState(StateType::eStateExited, true);
}
return;
}
@@ -662,10 +636,8 @@
case (SIGTRAP | (PTRACE_EVENT_EXIT << 8)): {
// The inferior process or one of its
[Lldb-commits] [PATCH] D41069: NPL: Clean up handling of inferior exit
labath added a comment. In https://reviews.llvm.org/D41069#951208, @clayborg wrote: > Is the lldb_private::Process we have an exit status in the class itself. The > first person to set the exit status wins and no one can set it twice. Doesn't > look like what we are doing here. I am not able to tell what actually fixes > things here? The `Process` class is in the client. The problem I am fixing is in server code, which was sending the `Wxx` packet multiple times. The happened because on linux we get one ptrace stop before the process dies (in this state its memory still exists, so we could still inspect it, if we wanted to) and then another one after its death (this happens after zombification of the process), and we were sending the packet after both. Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:439 -// The main thread exited. We're done monitoring. Report to delegate. -SetExitStatus(status, true); This is where the second packet got sent. Now it's the only place that sends death packets. Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:681 -if (is_main_thread) - SetExitStatus(WaitStatus::Decode(data), true); This is what sent the first packet (it eventually trickles down into GDBRemoteCommunicationServerLLGS::HandleInferiorState_Exited, which is what does the sending). Deleting it fixes things. https://reviews.llvm.org/D41069 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41069: NPL: Clean up handling of inferior exit
clayborg added a comment.
Is the lldb_private::Process we have an exit status in the class itself. The
first person to set the exit status wins and no one can set it twice. Doesn't
look like what we are doing here. I am not able to tell what actually fixes
things here?
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:43
TestClient::~TestClient() {
+ if( !IsConnected())
+return;
space after if and not after (
https://reviews.llvm.org/D41069
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41069: NPL: Clean up handling of inferior exit
labath created this revision.
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.
https://reviews.llvm.org/D41069
Files:
source/Plugins/Process/Linux/NativeProcessLinux.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
unittests/tools/lldb-server/tests/TestClient.cpp
Index: unittests/tools/lldb-server/tests/TestClient.cpp
===
--- unittests/tools/lldb-server/tests/TestClient.cpp
+++ unittests/tools/lldb-server/tests/TestClient.cpp
@@ -40,6 +40,9 @@
}
TestClient::~TestClient() {
+ if( !IsConnected())
+return;
+
std::string response;
// Debugserver (non-conformingly?) sends a reply to the k packet instead of
// simply closing the connection.
@@ -242,6 +245,18 @@
return E;
m_stop_reply = std::move(*creation);
+ if (!isa(m_stop_reply)) {
+StringExtractorGDBRemote R;
+PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
+if (result != PacketResult::ErrorDisconnected) {
+ return make_error(
+ formatv("Expected connection close after receiving {0}. Got {1}/{2} "
+ "instead.",
+ response, result, R.GetStringRef())
+ .str(),
+ inconvertibleErrorCode());
+}
+ }
return Error::success();
}
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -825,6 +825,7 @@
// We are ready to exit the debug monitor.
m_exit_now = true;
+ m_mainloop.RequestTermination();
}
void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(
Index: source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -412,46 +412,20 @@
// Handle when the thread exits.
if (exited) {
-LLDB_LOG(log, "got exit signal({0}) , tid = {1} ({2} main thread)", signal,
- pid, is_main_thread ? "is" : "is not");
+LLDB_LOG(log,
+ "got exit signal({0}) , tid = {1} ({2} main thread), process "
+ "state = {3}",
+ signal, pid, is_main_thread ? "is" : "is not", GetState());
// This is a thread that exited. Ensure we're not tracking it anymore.
-const bool thread_found = StopTrackingThread(pid);
+StopTrackingThread(pid);
if (is_main_thread) {
- // We only set the exit status and notify the delegate if we haven't
- // already set the process
- // state to an exited state. We normally should have received a SIGTRAP |
- // (PTRACE_EVENT_EXIT << 8)
- // for the main thread.
- const bool already_notified = (GetState() == StateType::eStateExited) ||
-(GetState() == StateType::eStateCrashed);
- if (!already_notified) {
-LLDB_LOG(
-log,
-"tid = {0} handling main thread exit ({1}), expected exit state "
-"already set but state was {2} instead, setting exit state now",
-pid,
-thread_found ? "stopped tracking thread metadata"
- : "thread metadata not found",
-GetState());
-// The main thread exited. We're done monitoring. Report to delegate.
-SetExitStatus(status, true);
+ // The main thread exited. We're done monitoring. Report to delegate.
+ SetExitStatus(status, true);
-// Notify delegate that our process has exited.
-SetState(StateType::eStateExited, true);
- } else
-LLDB_LOG(log, "tid = {0} main thread now exited (%s)", pid,
- thread_found ? "stopped tracking thread metadata"
- : "thread metadata not found");
-} else {
- // Do we want to
