Skip to content

Reapply "Fix error in unrecognized register name handling for "SBFram…e.register"" (#88468)" #88535

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 1 commit into from
Apr 12, 2024
Merged

Reapply "Fix error in unrecognized register name handling for "SBFram…e.register"" (#88468)" #88535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 11 additions & 1 deletion lldb/bindings/interface/SBFrameExtensions.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ STRING_EXTENSION_OUTSIDE(SBFrame)
def __init__(self, regs):
self.regs = regs

def __iter__(self):
return self.get_registers()

def get_registers(self):
for i in range(0,len(self.regs)):
rs = self.regs[i]
for j in range (0,rs.num_children):
Copy link
Member

Choose a reason for hiding this comment

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

The range expressions in this function can be simplified. Dropping the 0 means the list of numbers automatically starts at 0:

for i in range(len(self.regs)):
...
for j in range(rs.num_children):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless there's some difference in behavior, I don't mind burning a few characters to make the range explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Fair, explicit is good. I meant it as a suggestion originally.

reg = rs.GetChildAtIndex(j)
yield reg

def __getitem__(self, key):
if type(key) is str:
for i in range(0,len(self.regs)):
Expand All @@ -52,7 +62,7 @@ STRING_EXTENSION_OUTSIDE(SBFrame)
reg = rs.GetChildAtIndex(j)
if reg.name == key: return reg
else:
return lldb.SBValue()
return SBValue()

return registers_access(self.registers)

Expand Down
16 changes: 14 additions & 2 deletions lldb/test/API/python_api/frame/TestFrames.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,12 @@ def test_get_arg_vals_for_call_stack(self):
gpr_reg_set = lldbutil.get_GPRs(frame)
pc_value = gpr_reg_set.GetChildMemberWithName("pc")
self.assertTrue(pc_value, "We should have a valid PC.")
pc_value_int = int(pc_value.GetValue(), 0)


# Make sure on arm targets we dont mismatch PC value on the basis of thumb bit.
# Frame PC will not have thumb bit set in case of a thumb
# instruction as PC.
pc_value_int = int(pc_value.GetValue(), 0)
if self.getArchitecture() in ["arm", "armv7", "armv7k"]:
pc_value_int &= ~1
self.assertEqual(
Expand All @@ -91,7 +93,17 @@ def test_get_arg_vals_for_call_stack(self):
frame.GetSP(),
"SP gotten as a value should equal frame's GetSP",
)

# Test that the "register" property's flat list matches the list from
# the "registers" property that returns register sets:
register_regs = set()
flattened_regs = set()
for reg_set in frame.registers:
for reg in reg_set:
flattened_regs.add(reg.name)
for reg in frame.register:
register_regs.add(reg.name)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of iterating over it twice, why not combine into one loop?

Copy link
Collaborator Author

@jimingham jimingham Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Iterating over what twice? One is iterating over the elements of the list that is the registers property, and the other using the generator for the register (note no final s) property.

registers is actually a poorly named property since it actually produces a list of register sets. But this patch didn't add either property, so I can't really change the names.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now. I definitely did not notice the difference between frame.register and frame.registers... I wish we could change that. Maybe we could make frame.register_sets an alias for frame.registers and switch to using that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR is about fixing register not registers. If you try to iterate over it in the current sources you get a weird error. When I fixed just that error, then iterating over it spun forever, so I either had to explicitly turn off iterability or make some reasonable generator, which is what this patch does.

Adding register_sets as another property that does the same thing as registers seems fine, but also not really a part of this PR.

self.assertEqual(register_regs, flattened_regs, "register matches registers")

print("---", file=session)
process.Continue()

Expand Down