lemo updated this revision to Diff 114890.
lemo added a reviewer: markmentovai.
lemo added a comment.
Revised patch based on the review feedback: I looked into updating the running
state after everything else succeeds, but it proved a bit awkward since 1)
TrySetRunning() can fail and 2) if we check the running state then use
SetRunning() instead of TrySetRunning() it opens the possibility for a race.
For reference, here are the considered options:
1. Fix Process::Resume() so we only change the state late, after everything
succeeds
2. Rollback state change if anything fails while resuming
3. Patch the specific case of "resume is not supported"
4. Do nothing
The updated patch is using #2 - rollback the running state to "stopped" if the
resume operation fails. The downside is the possibility of a "partial rollback"
but from a cursory review of the code paths the risk seems no higher than
option #1. Thoughts?
I'm looking into adding a test case as well but I'm uploading the WIP patch to
get everyone a chance to take an early look.
Thanks!
Lemo.
https://reviews.llvm.org/D37651
Files:
source/Commands/CommandObjectThread.cpp
source/Plugins/Process/elf-core/ProcessElfCore.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -1621,7 +1621,12 @@
log->Printf("Process::Resume: -- TrySetRunning failed, not resuming.");
return error;
}
- return PrivateResume();
+ Status error = PrivateResume();
+ if (!error.Success()) {
+ // Undo running state change
+ m_public_run_lock.SetStopped();
+ }
+ return error;
}
Status Process::ResumeSynchronous(Stream *stream) {
@@ -1650,6 +1655,9 @@
error.SetErrorStringWithFormat(
"process not in stopped state after synchronous resume: %s",
StateAsCString(state));
+ } else {
+ // Undo running state change
+ m_public_run_lock.SetStopped();
}
// Undo the hijacking of process events...
Index: source/Plugins/Process/elf-core/ProcessElfCore.h
===================================================================
--- source/Plugins/Process/elf-core/ProcessElfCore.h
+++ source/Plugins/Process/elf-core/ProcessElfCore.h
@@ -89,6 +89,8 @@
//------------------------------------------------------------------
bool IsAlive() override;
+ bool WarnBeforeDetach() const override { return false; }
+
//------------------------------------------------------------------
// Process Memory
//------------------------------------------------------------------
Index: source/Commands/CommandObjectThread.cpp
===================================================================
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -94,7 +94,7 @@
bool all_threads = false;
if (command.GetArgumentCount() == 0) {
Thread *thread = m_exe_ctx.GetThreadPtr();
- if (!HandleOneThread(thread->GetID(), result))
+ if (!thread || !HandleOneThread(thread->GetID(), result))
return false;
return result.Succeeded();
} else if (command.GetArgumentCount() == 1) {
@@ -775,6 +775,12 @@
else
error = process->Resume();
+ if (!error.Success()) {
+ result.AppendMessage(error.AsCString());
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
+
// There is a race condition where this thread will return up the call
// stack to the main command handler
// and show an (lldb) prompt before HandlePrivateEvent (from
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -1621,7 +1621,12 @@
log->Printf("Process::Resume: -- TrySetRunning failed, not resuming.");
return error;
}
- return PrivateResume();
+ Status error = PrivateResume();
+ if (!error.Success()) {
+ // Undo running state change
+ m_public_run_lock.SetStopped();
+ }
+ return error;
}
Status Process::ResumeSynchronous(Stream *stream) {
@@ -1650,6 +1655,9 @@
error.SetErrorStringWithFormat(
"process not in stopped state after synchronous resume: %s",
StateAsCString(state));
+ } else {
+ // Undo running state change
+ m_public_run_lock.SetStopped();
}
// Undo the hijacking of process events...
Index: source/Plugins/Process/elf-core/ProcessElfCore.h
===================================================================
--- source/Plugins/Process/elf-core/ProcessElfCore.h
+++ source/Plugins/Process/elf-core/ProcessElfCore.h
@@ -89,6 +89,8 @@
//------------------------------------------------------------------
bool IsAlive() override;
+ bool WarnBeforeDetach() const override { return false; }
+
//------------------------------------------------------------------
// Process Memory
//------------------------------------------------------------------
Index: source/Commands/CommandObjectThread.cpp
===================================================================
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -94,7 +94,7 @@
bool all_threads = false;
if (command.GetArgumentCount() == 0) {
Thread *thread = m_exe_ctx.GetThreadPtr();
- if (!HandleOneThread(thread->GetID(), result))
+ if (!thread || !HandleOneThread(thread->GetID(), result))
return false;
return result.Succeeded();
} else if (command.GetArgumentCount() == 1) {
@@ -775,6 +775,12 @@
else
error = process->Resume();
+ if (!error.Success()) {
+ result.AppendMessage(error.AsCString());
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
+
// There is a race condition where this thread will return up the call
// stack to the main command handler
// and show an (lldb) prompt before HandlePrivateEvent (from
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
