Page MenuHomePhabricator

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

Authored by friss on Feb 6 2020, 12:58 PM.

Details

Summary

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

Diff Detail

Event Timeline

friss created this revision.Feb 6 2020, 12:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 12:58 PM
labath accepted this revision.Feb 6 2020, 1:39 PM

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).

This revision is now accepted and ready to land.Feb 6 2020, 1:39 PM
JDevlieghere accepted this revision.Feb 6 2020, 3:00 PM

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 added a subscriber: jingham.EditedFeb 6 2020, 6:35 PM

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 added a comment.Feb 6 2020, 8:13 PM

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?

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 updated this revision to Diff 243239.Feb 7 2020, 10:55 AM

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

friss added a comment.Feb 7 2020, 10:56 AM

I think we're on the same page then.

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 added inline comments.Feb 7 2020, 11:10 AM
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.

This revision was automatically updated to reflect the committed changes.