[lldb/API] NFC: Reformat SBThread::GetStopDescription()

[friss]

Summary

This gets rid of some nesting and of the raw char* variable that caused
the memory management bug Ismail hit yesterday.

[labath]

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

[JDevlieghere]

In D74157#1862537, @labath wrote:

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

+1, this doesn't seem like something that should be done at the API level. Otherwise LGTM.

[jingham]

In D74157#1862537, @labath wrote:

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

A thread that stops for no reason will have an empty StopInfoSP. So somebody has to check for that. I also don't want a thread that hasn't stopped for a reason to have a placeholder description like "no reason" because if you are using the API's you'd like to just print whatever the description is, and if you stop one thread at a breakpoint, having all the others say "no reason" is noisy and unhelpful.

But I agree that the SB layer is the wrong place to do this. SBThread::GetStopDescription should just call Thread->GetStopDescription and return whatever that returned. It really shouldn't even care whether the thread had a StopInfo or not, Thread::GetStopDescription should handle that case too.

I also really wish we didn't need the code to augment the Thread's stop description by using the stop reason. You only get to this code if there IS a StopInfo, so the StopInfo should have provided a description if there's a reason for stopping. Right now you can subclass StopInfo and return anything you want in GetStopDescription. So somebody could make a StopInfo subclass that represents a stop because we hit a signal - returning eStopReasonSignal from GetStopReason - and then just decide not to print anything for the description. But that's very unhelpful. And these reduced descriptions are a bit bogus (like any plan completed calls itself "step"...)

It would be really nice to just ban that. Maybe we could make the StopInfo virtual method be a DoGetStopDescription, and then have StopInfo::GetStopReason() not be virtual, and call DoGetStopDescription and assert if the StopReason in anything other than eStopReasonNone, but the description was empty. That way we could force people who handle the stops to provide useful stop reasons.

[friss]

In D74157#1863069, @jingham wrote:

In D74157#1862537, @labath wrote:

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

A thread that stops for no reason will have an empty StopInfoSP. So somebody has to check for that. I also don't want a thread that hasn't stopped for a reason to have a placeholder description like "no reason" because if you are using the API's you'd like to just print whatever the description is, and if you stop one thread at a breakpoint, having all the others say "no reason" is noisy and unhelpful.

I don't understand this comment. The fallback code we are talking about only ever triggers if there's a StopInfo. If the StopInfoSP was empty we wouldn't return a description.

But I agree that the SB layer is the wrong place to do this. SBThread::GetStopDescription should just call Thread->GetStopDescription and return whatever that returned. It really shouldn't even care whether the thread had a StopInfo or not, Thread::GetStopDescription should handle that case too.

I also really wish we didn't need the code to augment the Thread's stop description by using the stop reason. You only get to this code if there IS a StopInfo, so the StopInfo should have provided a description if there's a reason for stopping. Right now you can subclass StopInfo and return anything you want in GetStopDescription. So somebody could make a StopInfo subclass that represents a stop because we hit a signal - returning eStopReasonSignal from GetStopReason - and then just decide not to print anything for the description. But that's very unhelpful. And these reduced descriptions are a bit bogus (like any plan completed calls itself "step"...)

I removed this code and asserted that the description returned by StopInfos is not empty. The test suite passed fine. Do you have any example where this code would be needed? I think we should require that StopInfos return a non-empty description and get rid of it the fallback code.

It would be really nice to just ban that. Maybe we could make the StopInfo virtual method be a DoGetStopDescription, and then have StopInfo::GetStopReason() not be virtual, and call DoGetStopDescription and assert if the StopReason in anything other than eStopReasonNone, but the description was empty. That way we could force people who handle the stops to provide useful stop reasons.

Do you have any example how to get into a problematic case?

[jingham]

In D74157#1863126, @friss wrote:

In D74157#1863069, @jingham wrote:

In D74157#1862537, @labath wrote:

Looks better. TBH, I'm not sure why/if we really need the case handling the situation where the thread does not have a stop description. Ideally I'd just move this code there (or delete it).

A thread that stops for no reason will have an empty StopInfoSP. So somebody has to check for that. I also don't want a thread that hasn't stopped for a reason to have a placeholder description like "no reason" because if you are using the API's you'd like to just print whatever the description is, and if you stop one thread at a breakpoint, having all the others say "no reason" is noisy and unhelpful.

I don't understand this comment. The fallback code we are talking about only ever triggers if there's a StopInfo. If the StopInfoSP was empty we wouldn't return a description.

Sorry, I wasn't clear. I want SBThread::GetStopDescription to continue to return an empty string for threads with no reason. I wasn't sure whether folks were proposing having this always return something, and I don't think that's a good idea.

But I agree that the SB layer is the wrong place to do this. SBThread::GetStopDescription should just call Thread->GetStopDescription and return whatever that returned. It really shouldn't even care whether the thread had a StopInfo or not, Thread::GetStopDescription should handle that case too.

I also really wish we didn't need the code to augment the Thread's stop description by using the stop reason. You only get to this code if there IS a StopInfo, so the StopInfo should have provided a description if there's a reason for stopping. Right now you can subclass StopInfo and return anything you want in GetStopDescription. So somebody could make a StopInfo subclass that represents a stop because we hit a signal - returning eStopReasonSignal from GetStopReason - and then just decide not to print anything for the description. But that's very unhelpful. And these reduced descriptions are a bit bogus (like any plan completed calls itself "step"...)

I removed this code and asserted that the description returned by StopInfos is not empty. The test suite passed fine. Do you have any example where this code would be needed? I think we should require that StopInfos return a non-empty description and get rid of it the fallback code.

I agree.

It would be really nice to just ban that. Maybe we could make the StopInfo virtual method be a DoGetStopDescription, and then have StopInfo::GetStopReason() not be virtual, and call DoGetStopDescription and assert if the StopReason in anything other than eStopReasonNone, but the description was empty. That way we could force people who handle the stops to provide useful stop reasons.

Do you have any example how to get into a problematic case?

Since StopInfo::GetStopDescription is a virtual method, there's no guarantee that a child of StopInfo will always return a non-empty description, when it has a non-trivial StopReason. We put in defensive code to try to recover something useful out of that situation, but it would have been better to disallow it, I think.

[friss]

Remove the fallback code and assert that StopInfo returns a non-empty string.

[friss]

I think we're on the same page then.

In D74157#1864543, @jingham wrote:

Since StopInfo::GetStopDescription is a virtual method, there's no guarantee that a child of StopInfo will always return a non-empty description, when it has a non-trivial StopReason. We put in defensive code to try to recover something useful out of that situation, but it would have been better to disallow it, I think.

This commit adds an assert that the description is not empty. I think this is the best way to enforce the contract.

[jingham]

lldb/source/Target/Thread.cpp

601

It is possible to have a StopInfo with reason eStopReasonNone. I don't think we currently use that, but there's nothing outlawing it. We should treat that the same as no stop reason, so we shouldn't require a description in that case. Other than that this is fine.

lldb/source/API/SBThread.cpp

 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPlan.h"
 #include "lldb/Target/ThreadPlanStepInRange.h"
 #include "lldb/Target/ThreadPlanStepInstruction.h"
 #include "lldb/Target/ThreadPlanStepOut.h"
 #include "lldb/Target/ThreadPlanStepRange.h"
-#include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-enumerations.h"
 
 #include <memory>
 
 using namespace lldb;

 
 size_t SBThread::GetStopDescription(char *dst, size_t dst_len) {
   LLDB_RECORD_METHOD(size_t, SBThread, GetStopDescription, (char *, size_t), "",
                      dst_len);
 
   std::unique_lock<std::recursive_mutex> lock;
   ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
 
-  if (exe_ctx.HasThreadScope()) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&exe_ctx.GetProcessPtr()->GetRunLock())) {
-
-      StopInfoSP stop_info_sp = exe_ctx.GetThreadPtr()->GetStopInfo();
-      if (stop_info_sp) {
-        std::string thread_stop_desc =
-            exe_ctx.GetThreadPtr()->GetStopDescription();
-        const char *stop_desc = thread_stop_desc.c_str();
-
-        if (stop_desc[0] != '\0') {
-          if (dst)
-            return ::snprintf(dst, dst_len, "%s", stop_desc);
-          else {
-            // NULL dst passed in, return the length needed to contain the
-            // description
-            return ::strlen(stop_desc) + 1; // Include the NULL byte for size
-          }
-        } else {
-          size_t stop_desc_len = 0;
-          switch (stop_info_sp->GetStopReason()) {
-          case eStopReasonTrace:
-          case eStopReasonPlanComplete: {
-            static char trace_desc[] = "step";
-            stop_desc = trace_desc;
-            stop_desc_len =
-                sizeof(trace_desc); // Include the NULL byte for size
-          } break;
-
-          case eStopReasonBreakpoint: {
-            static char bp_desc[] = "breakpoint hit";
-            stop_desc = bp_desc;
-            stop_desc_len = sizeof(bp_desc); // Include the NULL byte for size
-          } break;
-
-          case eStopReasonWatchpoint: {
-            static char wp_desc[] = "watchpoint hit";
-            stop_desc = wp_desc;
-            stop_desc_len = sizeof(wp_desc); // Include the NULL byte for size
-          } break;
-
-          case eStopReasonSignal: {
-            stop_desc =
-                exe_ctx.GetProcessPtr()->GetUnixSignals()->GetSignalAsCString(
-                    stop_info_sp->GetValue());
-            if (stop_desc == nullptr || stop_desc[0] == '\0') {
-              static char signal_desc[] = "signal";
-              stop_desc = signal_desc;
-              stop_desc_len =
-                  sizeof(signal_desc); // Include the NULL byte for size
-            }
-          } break;
+    *dst = 0;
 
-          case eStopReasonException: {
-            char exc_desc[] = "exception";
-            stop_desc = exc_desc;
-            stop_desc_len = sizeof(exc_desc); // Include the NULL byte for size
-          } break;
+  if (!exe_ctx.HasThreadScope())
+    return 0;
 
-          case eStopReasonExec: {
-            char exc_desc[] = "exec";
-            stop_desc = exc_desc;
-            stop_desc_len = sizeof(exc_desc); // Include the NULL byte for size
-          } break;
+  Process::StopLocker stop_locker;
+  if (!stop_locker.TryLock(&exe_ctx.GetProcessPtr()->GetRunLock()))
+    return 0;
 
-          case eStopReasonThreadExiting: {
-            char limbo_desc[] = "thread exiting";
-            stop_desc = limbo_desc;
-            stop_desc_len = sizeof(limbo_desc);
-          } break;
-          default:
-            break;
-          }
+  std::string thread_stop_desc = exe_ctx.GetThreadPtr()->GetStopDescription();
+  if (thread_stop_desc.empty())
+    return 0;
 
-          if (stop_desc && stop_desc[0]) {
-            if (dst)
-              return ::snprintf(dst, dst_len, "%s", stop_desc) +
-                     1; // Include the NULL byte
+    return ::snprintf(dst, dst_len, "%s", thread_stop_desc.c_str()) + 1;
 
-            if (stop_desc_len == 0)
-              stop_desc_len = ::strlen(stop_desc) + 1; // Include the NULL byte
-
-            return stop_desc_len;
-          }
-        }
-      }
-    }
-  }
-  if (dst)
-    *dst = 0;
-  return 0;
+  // NULL dst passed in, return the length needed to contain the
+  // description.
+  return thread_stop_desc.size() + 1; // Include the NULL byte for size
 }
 
 SBValue SBThread::GetStopReturnValue() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBValue, SBThread, GetStopReturnValue);
 
   ValueObjectSP return_valobj_sp;
   std::unique_lock<std::recursive_mutex> lock;
   ExecutionContext exe_ctx(m_opaque_sp.get(), lock);

lldb/source/Target/Thread.cpp

if (!recognized_stop_description.empty())
     return recognized_stop_description;
 
   return GetStopDescriptionRaw();
 }
 
 std::string Thread::GetStopDescriptionRaw() {
   StopInfoSP stop_info_sp = GetStopInfo();
   std::string raw_stop_description;
-  if (stop_info_sp && stop_info_sp->IsValid())
+  if (stop_info_sp && stop_info_sp->IsValid()) {
     raw_stop_description = stop_info_sp->GetDescription();
+    assert((!raw_stop_description.empty() ||
+            stop_info_sp->GetStopReason() == eStopReasonNone) &&
+           "StopInfo returned an empty description.");
+  }
   return raw_stop_description;
 }
 
 void Thread::SelectMostRelevantFrame() {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD);
 
   auto frames_list_sp = GetStackFrameList();