jingham added a comment. In D74157#1863126 <https://reviews.llvm.org/D74157#1863126>, @friss wrote:
> 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. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74157/new/ https://reviews.llvm.org/D74157 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
