Page MenuHomePhabricator

Add an assertion for frame[0] being valid in CommandObjectThread.cpp.
ClosedPublic

Authored by sas on Mar 23 2015, 7:27 PM.

Details

Summary

This should always be true but sometimes is not, during platform bring
up. As recommended by Jim Ingham, an assertion should be enough here to
help.
This addresses post commit comments in http://reviews.llvm.org/D8554.

Diff Detail

Repository
rL LLVM

Event Timeline

sas updated this revision to Diff 22537.Mar 23 2015, 7:27 PM
sas retitled this revision from to Use eFlagRequiresFrame instead of manually checking in CommandObjectThread.cpp..
sas updated this object.
sas edited the test plan for this revision. (Show Details)
sas added reviewers: jasonmolenda, emaste, jingham, clayborg.
sas added a subscriber: Unknown Object (MLST).
jasonmolenda accepted this revision.Mar 23 2015, 8:20 PM
jasonmolenda edited edge metadata.

Good change as long as it accomplishes the same thing as your null check against GetStackFrameAtIndex(0). eFlagRequiresFrame is slightly different, checking that the ExecutionContext got a frame but I suppose the ECX got it from the same place -- GetStackFrameAtIndex(0) so it should be fine.

I wouldn't raise such a pedantic point except that lldb is clearly in an odd state when this code path is erroring out; normal assumptions may not be safe.

This revision is now accepted and ready to land.Mar 23 2015, 8:20 PM
sas added a comment.Mar 23 2015, 9:43 PM

Agreed. The only small benefit of this patch is to help a bit during target bring up.

clayborg accepted this revision.Mar 24 2015, 9:38 AM
clayborg edited edge metadata.

eFlagRequiresFrame is fine as long as you are looking for the selected target, process, thread and frame.

jingham edited edge metadata.Mar 24 2015, 10:47 AM

The only thing that bothers me a bit about this patch is that somebody might think we need to go clean up all the places where a thread doesn't have a frame 0, and worse, actually try to do something reasonable in that case. That would be wasted effort. To that end, an assert here might in fact be a better solution. In any case, it might be good to put a comment in Thread.h making it clear that GetFrameAtIndex(0) is always expected to return a valid frame.

Jim

emaste edited edge metadata.Mar 24 2015, 11:41 AM

To that end, an assert here might in fact be a better solution.

That would be fine with me, and would have helped in the specific case I encountered during bring-up.

sas added a comment.Mar 25 2015, 10:57 AM

Alright, let's go for the assert then.

sas updated this revision to Diff 22659.Mar 25 2015, 11:00 AM
sas edited edge metadata.

Use an assertion instead.

sas retitled this revision from Use eFlagRequiresFrame instead of manually checking in CommandObjectThread.cpp. to Add an assertion for frame[0] being valid in CommandObjectThread.cpp..Mar 25 2015, 11:01 AM
sas updated this object.
emaste accepted this revision.Mar 25 2015, 12:36 PM
emaste edited edge metadata.
This revision was automatically updated to reflect the committed changes.