Skip to content

[lldb][nfc] Factor out code checking if Variable is in scope #143572

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 3 commits into from
Jun 11, 2025
Merged

[lldb][nfc] Factor out code checking if Variable is in scope #143572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9821fd7
[lldb][nfc] Factor out code checking if Variable is in scope
felipepiovezan Jun 10, 2025
2fe9bb4
fixup! pass by const reference
felipepiovezan Jun 10, 2025
7a15102
fixup! pass by const reference
felipepiovezan Jun 10, 2025
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
3 changes: 3 additions & 0 deletions lldb/include/lldb/Symbol/Variable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ class Variable : public UserID, public std::enable_shared_from_this<Variable> {

bool IsInScope(StackFrame *frame);

/// Returns true if this variable is in scope at `addr` inside `block`.
bool IsInScope(Block &block, Address addr);

bool LocationIsValidForFrame(StackFrame *frame);

bool LocationIsValidForAddress(const Address &address);
Expand Down
46 changes: 24 additions & 22 deletions lldb/source/Symbol/Variable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,28 +290,9 @@ bool Variable::IsInScope(StackFrame *frame) {
// this variable was defined in is currently
Block *deepest_frame_block =
frame->GetSymbolContext(eSymbolContextBlock).block;
if (deepest_frame_block) {
SymbolContext variable_sc;
CalculateSymbolContext(&variable_sc);

// Check for static or global variable defined at the compile unit
// level that wasn't defined in a block
if (variable_sc.block == nullptr)
return true;

// Check if the variable is valid in the current block
if (variable_sc.block != deepest_frame_block &&
!variable_sc.block->Contains(deepest_frame_block))
return false;

// If no scope range is specified then it means that the scope is the
// same as the scope of the enclosing lexical block.
if (m_scope_range.IsEmpty())
return true;

addr_t file_address = frame->GetFrameCodeAddress().GetFileAddress();
return m_scope_range.FindEntryThatContains(file_address) != nullptr;
}
Address frame_addr = frame->GetFrameCodeAddress();
if (deepest_frame_block)
return IsInScope(*deepest_frame_block, frame_addr);
}
break;

Expand All @@ -321,6 +302,27 @@ bool Variable::IsInScope(StackFrame *frame) {
return false;
}

bool Variable::IsInScope(Block &block, Address addr) {
Copy link
Member

@JDevlieghere JDevlieghere Jun 10, 2025

Choose a reason for hiding this comment

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

Any reason to not pass this by const ref?

Copy link
Contributor Author

@felipepiovezan felipepiovezan Jun 10, 2025

Choose a reason for hiding this comment

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

Just an oopsie on my part!

Copy link
Member

@JDevlieghere JDevlieghere Jun 10, 2025

Choose a reason for hiding this comment

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

What about Address? :D

Copy link
Contributor Author

@felipepiovezan felipepiovezan Jun 10, 2025

Choose a reason for hiding this comment

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

That one is just a pointer + integer, my understanding is that it is supposed to be passed by value?

Copy link
Contributor Author

@felipepiovezan felipepiovezan Jun 10, 2025

Choose a reason for hiding this comment

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

Mmmm it is a weak pointer + integer. How cheap is it to copy a weak pointer?

Copy link
Contributor Author

@felipepiovezan felipepiovezan Jun 10, 2025

Choose a reason for hiding this comment

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

Like std::shared_ptr, a typical implementation of weak_ptr stores two pointers:
a pointer to the control block; and
the stored pointer of the shared_ptr it was constructed from.

Seems like it is cheap?

Copy link
Member

@JDevlieghere JDevlieghere Jun 10, 2025

Choose a reason for hiding this comment

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

A quick grep shows 223 instances of it being passed by const ref and the class itself seems to be taking other Addresses by const-ref too. There are definitely a bunch of instances of it being passed by value, but I would say "it's only slightly more expensive" isn't a strong argument unless there's a benefit to weigh that against such as needing a copy to modify or something. My final argument would be that if we ever do address spaces, the class will grow, which makes the case for the reference more compelling.

Copy link
Contributor Author

@felipepiovezan felipepiovezan Jun 10, 2025

Choose a reason for hiding this comment

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

That's an interesting, given the current usage it makes sense to switch this to const reference.

Future changes aside, it's still worth pointing out that "by value" by default is usually the correct choice. An API that prescribes passing something by reference is also saying "it's a bad idea to copy this object", a warning to users of the API. Here, there is no such warning, so it is an API that makes a developer pause for nothing -- the warnings are usually "it's expensive", "you can't copy because of ownership semantics".

SymbolContext variable_sc;
CalculateSymbolContext(&variable_sc);

// Check for static or global variable defined at the compile unit
// level that wasn't defined in a block
if (variable_sc.block == nullptr)
return true;

// Check if the variable is valid in the current block
if (variable_sc.block != &block && !variable_sc.block->Contains(&block))
return false;

// If no scope range is specified then it means that the scope is the
// same as the scope of the enclosing lexical block.
if (m_scope_range.IsEmpty())
return true;

return m_scope_range.FindEntryThatContains(addr.GetFileAddress()) != nullptr;
}

Status Variable::GetValuesForVariableExpressionPath(
llvm::StringRef variable_expr_path, ExecutionContextScope *scope,
GetVariableCallback callback, void *baton, VariableList &variable_list,
Expand Down
Loading