Skip to content

[lldb] Add DWARFExpressionEntry and GetExpressionEntryAtAddress() to … #144238

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
JDevlieghere merged 15 commits into from
Jul 12, 2025
Merged

[lldb] Add DWARFExpressionEntry and GetExpressionEntryAtAddress() to … #144238

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8ed8c54
[lldb] Add DWARFExpressionEntry and GetExpressionEntryAtAddress() to …
UltimateForce21 Jun 11, 2025
1db5002
Update lldb/include/lldb/Expression/DWARFExpressionList.h
UltimateForce21 Jun 19, 2025
a26010b
Update lldb/include/lldb/Expression/DWARFExpressionList.h
UltimateForce21 Jun 19, 2025
72237b7
Update lldb/source/Expression/DWARFExpressionList.cpp
UltimateForce21 Jun 19, 2025
94e4951
Update DWARFExpressionList.h
UltimateForce21 Jun 24, 2025
e8142da
Update DWARFExpressionList.cpp
UltimateForce21 Jun 24, 2025
7e8741e
Update DWARFExpressionList.h
UltimateForce21 Jun 28, 2025
c4cd77f
Update DWARFExpressionList.cpp
UltimateForce21 Jun 28, 2025
62c02a9
Change GetExpressionEntryAtAddress to return std::optional instead of…
UltimateForce21 Jun 29, 2025
d015971
Update DWARFExpressionList.cpp
UltimateForce21 Jul 2, 2025
60898ea
Add underflow/overflow checks to GetExpressionEntryAtAddressi
UltimateForce21 Jul 3, 2025
3462165
Make file_range optional in DWARFExpressionEntry for always-valid expr
UltimateForce21 Jul 8, 2025
c01a747
Merge branch 'main' into add-dwarfexprentry-api
UltimateForce21 Jul 9, 2025
ec3ac93
Merge branch 'main' into add-dwarfexprentry-api
UltimateForce21 Jul 10, 2025
38baa90
Fixed formating issues
UltimateForce21 Jul 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
12 changes: 12 additions & 0 deletions lldb/include/lldb/Expression/DWARFExpressionList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,18 @@ class DWARFExpressionList {

lldb::addr_t GetFuncFileAddress() { return m_func_file_addr; }

/// Represents an entry in the DWARFExpressionList with all needed metadata
struct DWARFExpressionEntry {
lldb::addr_t base;
lldb::addr_t end;
Copy link
Member

@JDevlieghere JDevlieghere Jun 16, 2025

Choose a reason for hiding this comment

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

Are these file or load addresses? If you use lldb::addr_t (as opposed to the Address class), it's always a good idea to make that part of the variable name (like you did for func_load_addr) or at least add a Doxygen-style comment.

Copy link
Collaborator

@adrian-prantl adrian-prantl Jun 16, 2025

Choose a reason for hiding this comment

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

Note that if you do want to use Address, there is already an AddressRange class.

Copy link
Contributor Author

@UltimateForce21 UltimateForce21 Jun 19, 2025

Choose a reason for hiding this comment

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

These are file addresses (without offsets applied), I will try to make that more clear both in the comments and in the variable names. I'll also take a look into the AddressRange class and see if I can utilize it, thank you for the pointers.

const DWARFExpression *expr;
};

/// Returns the entry (base, end, data) for a given PC address
llvm::Expected<DWARFExpressionEntry>
GetExpressionEntryAtAddress(lldb::addr_t func_load_addr,
lldb::addr_t load_addr) const;

const DWARFExpression *GetExpressionAtAddress(lldb::addr_t func_load_addr,
lldb::addr_t load_addr) const;

Expand Down
21 changes: 21 additions & 0 deletions lldb/source/Expression/DWARFExpressionList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,27 @@ bool DWARFExpressionList::ContainsAddress(lldb::addr_t func_load_addr,
return GetExpressionAtAddress(func_load_addr, addr) != nullptr;
}

llvm::Expected<DWARFExpressionList::DWARFExpressionEntry>
DWARFExpressionList::GetExpressionEntryAtAddress(lldb::addr_t func_load_addr,
lldb::addr_t load_addr) const {
if (const DWARFExpression *expr = GetAlwaysValidExpr()) {
return DWARFExpressionEntry{0, LLDB_INVALID_ADDRESS, expr};
}

if (func_load_addr == LLDB_INVALID_ADDRESS)
func_load_addr = m_func_file_addr;

addr_t addr = load_addr - func_load_addr + m_func_file_addr;
uint32_t index = m_exprs.FindEntryIndexThatContains(addr);
if (index == UINT32_MAX) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No DWARF expression found for address 0x%llx", addr);
Copy link
Collaborator

@adrian-prantl adrian-prantl Jun 16, 2025

Choose a reason for hiding this comment

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

Generally using Expected is desirable, especially if there is actionable information in the error message (and information that only this function can provide). Lookups are borderline cases. If it's expected during normal operation that client will call this API and the lookup fails, then I would return a std::optional instead. The client can turn this into a more targeted error message if needed.
In short — I would return a std::optional here.

Copy link
Contributor Author

@UltimateForce21 UltimateForce21 Jun 19, 2025

Choose a reason for hiding this comment

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

Sounds good, I will refactor it to return std::optional instead.

}

const Entry &entry = *m_exprs.GetEntryAtIndex(index);
return DWARFExpressionEntry{entry.base, entry.GetRangeEnd(), &entry.data};
}

const DWARFExpression *
DWARFExpressionList::GetExpressionAtAddress(lldb::addr_t func_load_addr,
Copy link
Collaborator

@adrian-prantl adrian-prantl Jun 16, 2025

Choose a reason for hiding this comment

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

Out of curiosity: Do we still need this API, or is it just a subset of the function above?

Copy link
Contributor Author

@UltimateForce21 UltimateForce21 Jun 19, 2025

Choose a reason for hiding this comment

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

As you know the new function GetExpressionEntryAtAddress, does almost exact same thing as this one. I don't see this function being used anywhere else in the lldb code base, so maybe it can be removed, but to be safe I did not want to alter it. But from what I can tell, it should be okay to remove it. We can also choose to keep it and maybe refactor it to just call the GetExpressionEntryAtAddress and just return the DWARFExpression *expr.

Copy link
Collaborator

@adrian-prantl adrian-prantl Jun 21, 2025

Choose a reason for hiding this comment

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

If it's completely unused, and there is no indication that was added in support of any downstream clients (either as a comment or in the commit message) we could consider deleting it in a separate PR. Not urgent.

lldb::addr_t load_addr) const {
Expand Down