This is an archive of the discontinued LLVM Phabricator instance.

Thread::GetStackFrameCount should forestall interruption
ClosedPublic

Authored by jingham on May 9 2023, 5:08 PM.

Details

Summary

We can't let GetStackFrameCount get interrupted or it will give the wrong answer. Plus, it's useful in some places to have a way to force the full stack to be created even in the face of interruption. Moreover, most of the time when you're just getting frames, you don't need to know the number of frames in the stack to start with. You just keep calling Thread::GetStackFrameAtIndex(index++) and when you get a null StackFrameSP back, you're done. That's also more amenable to interruption if you are doing some work frame by frame.

So this patch makes GetStackFrameCount always return the full count, suspending interruption. I also went through all the places that use GetStackFrameCount to make sure that they really needed the full stack walk. In many cases, they did not. For instance frame select -r 10 was getting the number of frames just to check whether cur_frame_idx + 10 was within the stack. It's better in that case to see if that frame exists first, since that doesn't force a full stack walk, and only deal with walking off the end of the stack if it doesn't...

I also added a test for some of these behaviors.

Diff Detail

Event Timeline

jingham created this revision.May 9 2023, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 5:08 PM
jingham requested review of this revision.May 9 2023, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 5:08 PM
bulbazord added inline comments.May 9 2023, 11:15 PM
lldb/include/lldb/Target/StackFrameList.h
103–104

Is the range inclusive of end_idx? as in, is it [0, end_idx) or [0, end_idx]?

lldb/source/Target/StackFrameList.cpp
89

nit:

GetFramesUpTo(/*end_idx = */0, /*allow_interrupt = */false);

Or something like this... A little easier to understand IMO.

640–641

Same here I think.

683–684

nit: Since interrupted isn't used anywhere else, you could do something like:

if (bool interrupted = GetFramesUpTo(idx)) {
 // Code here
}

You could also just do if (GetFramesUpTo(idx)) but I think the name of the function isn't descriptive enough to do that and stay readable.

lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
17

I have a feeling this docstring was supposed to be different?

mib requested changes to this revision.May 10 2023, 11:37 AM

I don't understand why the succeeding return value for GetFramesUpTo is false. It looks counter-intuitive to me. What's the motivation behind that ?

lldb/include/lldb/Target/StackFrameList.h
103–104

@bulbazord I believe this should be inclusive.

@jingham This comment sounds like we only return true if the function was interrupted, which is not the expected behavior, right ?

lldb/source/Target/StackFrameList.cpp
448

I find it confusing the fail here given that we've succeeded at fetching the frames

633

Again, I believe this should return true since we succeeded at retrieving all the requested frames.

683–684

+1: reading if (GetFramesUpTo(idx)), I'd never think that the succeeding result would be false.

lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
47

Is this necessary if you already have it in the cleanup method ?

This revision now requires changes to proceed.May 10 2023, 11:37 AM
jingham added inline comments.May 10 2023, 5:50 PM
lldb/include/lldb/Target/StackFrameList.h
103–104

The function used to return void, so it had no expected behavior before. I am defining the "bool" return here, and I define it to be "was_interrupted" as I say in the doc string. That's also why the return values you are questioning below are actually correct. This is NOT returning "success" it is returning "interrupted".

lldb/source/Target/StackFrameList.cpp
448

GetFramesUpTo returns true if interrupted, false if not. This was not interrupted, so it should return false. I could switch this to "true if not interrupted" so that I could return true here, but that would be weird. This isn't a "success or fail" result, it's an "interrupted or not interrupted" result.

jingham updated this revision to Diff 521183.May 10 2023, 5:53 PM

Address review comments

JDevlieghere added inline comments.May 11 2023, 4:16 AM
lldb/include/lldb/Target/StackFrameList.h
106

I personally would prefer to have an InterruptPolicy (e.g. AllowInterrupt, DenyInterrupt) to limit the proliferation of boolean flags and improve readability.

lldb/source/Target/StackFrameList.cpp
513–514

Nit: you could inline his and still fit in a single line (probably).

Long term I think we should have an API in Debugger that allows you to report who was interrupted and then we can centralize this logging + have a way for the API tests to request that info (rather than having to parse the logs).

mib added inline comments.May 11 2023, 10:34 AM
lldb/include/lldb/Target/StackFrameList.h
103–104

Given that this changes the behavior of GetFramesUpTo, I'd rather pass a bool reference that says is_interrupted and keep void as the return type, because if (GetFramesUpTo(idx, true) == false) really doesn't tell me much. Looking at this code, I'd think why did GetFramesUpTo failed.

106

+1, this would greatly improve readability

jingham updated this revision to Diff 521380.May 11 2023, 11:15 AM

Address Review Comments

jingham marked 2 inline comments as done.May 11 2023, 11:22 AM

I added the enum, that's a good idea.

I agree that we should centralize internal reporting of Interrupt events, but as your "longer term" suggests, that's a design task and orthogonal to this patch. I'll do that in a subsequent patch.

lldb/include/lldb/Target/StackFrameList.h
103–104

I still don't see this. GetFramesUpTo says clearly what the return value is, so I don't see why this is confusing. You would only thing "why did GetFramesUpTo fail" if you didn't read the function definition.

Moreover, the code you are looking at will always do:

if (GetFramesUpTo(idx, AllowInterruption)) {

// Do something that's explicitly handling interruption

}

so it should be pretty clear what is going on.

I don't think adding another parameter is warranted. You would have to pass it even it you passed DoNotAllowInterruption, which is weird. Or we could take a bool * but that's just more code to no very good purpose, IMO.

mib accepted this revision.May 11 2023, 11:32 AM

I guess the InterruptionControl argument makes it clearer. LGTM

This revision is now accepted and ready to land.May 11 2023, 11:32 AM