[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Leonard Mosescu via Phabricator via lldb-commits]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37651.114890.patch
Type: text/x-patch
Size: 2405 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170912/a5d48887/attachment-0001.bin>