Skip to content

Don't count all the frames just to skip the current inlined ones. #80918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 13, 2024
Merged

Don't count all the frames just to skip the current inlined ones. #80918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c76edee
Don't count all the frames just to skip the current inlined ones.
jimingham Feb 7, 2024
e8659a1
Add a comment about Thread::GetStackFrameCount being expensive.
jimingham Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lldb/include/lldb/Target/Thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,13 @@ class Thread : public std::enable_shared_from_this<Thread>,
/// and having the thread call the SystemRuntime again.
virtual bool ThreadHasQueueInformation() const { return false; }

/// GetStackFrameCount can be expensive. Stacks can get very deep, and they
/// require memory reads for each frame. So only use GetStackFrameCount when
/// you need to know the depth of the stack. When iterating over frames, its
/// better to generate the frames one by one with GetFrameAtIndex, and when
/// that returns NULL, you are at the end of the stack. That way your loop
/// will only do the work it needs to, without forcing lldb to realize
/// StackFrames you weren't going to look at.
virtual uint32_t GetStackFrameCount() {
return GetStackFrameList()->GetNumFrames();
}
Expand Down
7 changes: 3 additions & 4 deletions lldb/source/Expression/DWARFExpression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,11 +608,10 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
StackFrameSP parent_frame = nullptr;
addr_t return_pc = LLDB_INVALID_ADDRESS;
uint32_t current_frame_idx = current_frame->GetFrameIndex();
uint32_t num_frames = thread->GetStackFrameCount();
for (uint32_t parent_frame_idx = current_frame_idx + 1;
parent_frame_idx < num_frames; ++parent_frame_idx) {

for (uint32_t parent_frame_idx = current_frame_idx + 1;;parent_frame_idx++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: If you initialize parent_frame to thread->GetStackFrameAtIndex(current_frame_idx + 1) and move the parent_frame = ... bit to the end of the loop, you can have the loop condition be parent_frame != nullptr instead of relying on a break statement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I would second that suggestion to make the code easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this, but I don't think it makes things any clearer. This isn't a simple loop, it has another break, and a continue. You end up having to cache IsInline before you reset the parent_frame and that hides where the important next frame part of the code goes. That's just awkward.
I think this version is clearer. The first thing the loop does is fetch the next frame and, checks if it is null as the signal that the stack walk is done. The way I wrote it keeps those operations right next to one another which I think is easier to read..

parent_frame = thread->GetStackFrameAtIndex(parent_frame_idx);
// Require a valid sequence of frames.
// If this is null, we're at the end of the stack.
if (!parent_frame)
break;

Expand Down