[Lldb-commits] [PATCH] D74157: [lldb/API] NFC: Reformat SBThread::GetStopDescription()

Thu Feb 6 20:15:52 PST 2020

friss added a comment.

In D74157#1863069 <https://reviews.llvm.org/D74157#1863069>, @jingham wrote:

> In D74157#1862537 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74157/new/

https://reviews.llvm.org/D74157