[Windows, Process] Fix an issue in windows thread handling that was causing LLDB to hang

[stella.stamenova]

Summary

The function ResumeThread on Windows returns a DWORD which is an unsigned int. In TargetThreadWindows::DoResume, there's code that determines how many times to call ResumeThread based on whether the return value is greater than 0. Since the function returns -1 (as an unsigned int) on failure, this was getting stuck in an infinite loop if ResumeThread failed for any reason. The correct thing to do is check whether the return value is -1 and then return the appropriate error instead of ignoring the return value.

[zturner]

source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp

127–128 ↗

(On Diff #147344)

Can you move this check inside the loop and then remove the additional check from the while condition?

Also, can you write == (DWORD)-1 or == 0xFFFFFFFF instead of == -1? It's the same meaning, but it makes it slightly clearer to the reader that this is intentional.

[zturner]

Ahh it looks like you attached the updated patch directly to this email instead of updating it through Phabricator. If you can updated it through the Phabricator UI it's more helpful since it updates the graphical diff viewer that way. Otherwise have to open the patch in a separated text editor.

In any case, lgtm.

[stella.stamenova]

I used Phabricator, actually, I am not sure why it came out differently.

[zturner]

Yea, was just user error on my part. I was looking at the wrong thing.

lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

if (active_exception) {
       // exception so that from the process's view, there is no indication that
       // anything happened.
       m_session_data->m_debugger->ContinueAsyncException(
           ExceptionResult::MaskException);
     }
 
     LLDB_LOG(log, "resuming {0} threads.", m_thread_list.GetSize());
 
+    bool failed = false;
     for (uint32_t i = 0; i < m_thread_list.GetSize(); ++i) {
       auto thread = std::static_pointer_cast<TargetThreadWindows>(
           m_thread_list.GetThreadAtIndex(i));
-      thread->DoResume();
+      Status result = thread->DoResume();
+      if (result.Fail()) {
+        failed = true;
+        LLDB_LOG(log, "Trying to resume thread at index {0}, but failed with error {1}.", i, result);
+      }
     }
 
+    if (failed) {
+      error.SetErrorString("ProcessWindows::DoResume failed");
+      return error;
+    } else {
       SetPrivateState(eStateRunning);
+    }
   } else {
     LLDB_LOG(log, "error: process %I64u is in state %u.  Returning...",
              m_session_data->m_debugger->GetProcess().GetProcessId(),
              GetPrivateState());
   }
   return error;
 }
 

lldb/trunk/source/Plugins/Process/Windows/Common/TargetThreadWindows.h

public:
   void WillResume(lldb::StateType resume_state) override;
   void DidStop() override;
   lldb::RegisterContextSP GetRegisterContext() override;
   lldb::RegisterContextSP
   CreateRegisterContextForFrame(StackFrame *frame) override;
   bool CalculateStopInfo() override;
   Unwind *GetUnwinder() override;
 
-  bool DoResume();
+  Status DoResume();
 
   HostThread GetHostThread() const { return m_host_thread; }
 
 private:
   lldb::RegisterContextSP CreateRegisterContextForFrameIndex(uint32_t idx);
 
   HostThread m_host_thread;
 };
 }
 
 #endif

lldb/trunk/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp

 Unwind *TargetThreadWindows::GetUnwinder() {
   // FIXME: Implement an unwinder based on the Windows unwinder exposed through
   // DIA SDK.
   if (m_unwinder_ap.get() == NULL)
     m_unwinder_ap.reset(new UnwindLLDB(*this));
   return m_unwinder_ap.get();
 }
 
-bool TargetThreadWindows::DoResume() {
+Status TargetThreadWindows::DoResume() {
   StateType resume_state = GetTemporaryResumeState();
   StateType current_state = GetState();
   if (resume_state == current_state)
-    return true;
+    return Status();
 
   if (resume_state == eStateStepping) {
     uint32_t flags_index =
         GetRegisterContext()->ConvertRegisterKindToRegisterNumber(
             eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FLAGS);
     uint64_t flags_value =
         GetRegisterContext()->ReadRegisterAsUnsigned(flags_index, 0);
     flags_value |= 0x100; // Set the trap flag on the CPU
     GetRegisterContext()->WriteRegisterFromUnsigned(flags_index, flags_value);
   }
 
   if (resume_state == eStateStepping || resume_state == eStateRunning) {
     DWORD previous_suspend_count = 0;
     HANDLE thread_handle = m_host_thread.GetNativeThread().GetSystemHandle();
     do {
+      // ResumeThread returns -1 on error, or the thread's *previous* suspend count on success.
+      // This means that the return value is 1 when the thread was restarted.
+      // Note that DWORD is an unsigned int, so we need to explicitly compare with -1.
       previous_suspend_count = ::ResumeThread(thread_handle);
-    } while (previous_suspend_count > 0);
+
+      if (previous_suspend_count == (DWORD)-1)
+        return Status(::GetLastError(), eErrorTypeWin32);
+
+    } while (previous_suspend_count > 1);
   }
-  return true;
+
+  return Status();
 }