Reapply "Fix error in unrecognized register name handling for "SBFram…e.register"" (#88468)" #88535
Conversation
…e.register"" (llvm#88468)" with a fix for the "register" iterator test to not rely on particular register names. I mistook where the artificial "pc" register is generated. It isn't added to the register list or the register sets (except on arm where that's the name of the actual register), so I can't use it in this test. I instead just assert that the "register" generator produces the same list as flattening the register sets from "registers". This reverts commit 9f14914.
jimingham
requested review from
chelcassanova and
bulbazord
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesThe only change is a fix for the "register" iterator test to not rely on particular register names. I mistook where the artificial "pc" register is generated. It isn't added to the register list or the register sets (except on arm where that's the name of the actual register), so I can't use it in this test. I instead just assert that the "register" generator produces the same list as flattening the register sets from "registers". This reverts commit 9f14914. Full diff: https://github.com/llvm/llvm-project/pull/88535.diff 2 Files Affected:
diff --git a/lldb/bindings/interface/SBFrameExtensions.i b/lldb/bindings/interface/SBFrameExtensions.i
index 43b22ed7a6b325..e0472280666ab9 100644
--- a/lldb/bindings/interface/SBFrameExtensions.i
+++ b/lldb/bindings/interface/SBFrameExtensions.i
@@ -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):
+ reg = rs.GetChildAtIndex(j)
+ yield reg
+
def __getitem__(self, key):
if type(key) is str:
for i in range(0,len(self.regs)):
@@ -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)
diff --git a/lldb/test/API/python_api/frame/TestFrames.py b/lldb/test/API/python_api/frame/TestFrames.py
index a82b129bc8099d..0ec12206024ab7 100644
--- a/lldb/test/API/python_api/frame/TestFrames.py
+++ b/lldb/test/API/python_api/frame/TestFrames.py
@@ -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(
@@ -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)
+ self.assertEqual(register_regs, flattened_regs, "register matches registers")
+
print("---", file=session)
process.Continue()
|
You can test this locally with the following command:darker --check --diff -r e0a628715a8464e220c8ba9e9aaaf2561139198a...4a335c835239edc1989a1ddd7875a6e6edd2d143 lldb/test/API/python_api/frame/TestFrames.pyView the diff from darker here.--- TestFrames.py 2024-04-12 17:00:07.000000 +0000
+++ TestFrames.py 2024-04-12 17:09:04.374356 +0000
@@ -71,12 +71,11 @@
# but they should be valid. Uses get_GPRs() from the lldbutil
# module.
gpr_reg_set = lldbutil.get_GPRs(frame)
pc_value = gpr_reg_set.GetChildMemberWithName("pc")
self.assertTrue(pc_value, "We should have a valid PC.")
-
-
+
# 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"]:
@@ -100,12 +99,14 @@
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)
- self.assertEqual(register_regs, flattened_regs, "register matches registers")
-
+ self.assertEqual(
+ register_regs, flattened_regs, "register matches registers"
+ )
+
print("---", file=session)
process.Continue()
# At this point, the inferior process should have exited.
self.assertEqual(process.GetState(), lldb.eStateExited, PROCESS_EXITED)
|
| for reg in reg_set: | ||
| flattened_regs.add(reg.name) | ||
| for reg in frame.register: | ||
| register_regs.add(reg.name) |
There was a problem hiding this comment.
Instead of iterating over it twice, why not combine into one loop?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| def get_registers(self): | ||
| for i in range(0,len(self.regs)): | ||
| rs = self.regs[i] | ||
| for j in range (0,rs.num_children): |
There was a problem hiding this comment.
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):
There was a problem hiding this comment.
Unless there's some difference in behavior, I don't mind burning a few characters to make the range explicit.
There was a problem hiding this comment.
Fair, explicit is good. I meant it as a suggestion originally.
……e.register"" (llvm#88468)" (llvm#88535) The only change is a fix for the "register" iterator test to not rely on particular register names. I mistook where the artificial "pc" register is generated. It isn't added to the register list or the register sets (except on arm where that's the name of the actual register), so I can't use it in this test. I instead just assert that the "register" generator produces the same list as flattening the register sets from "registers". This reverts commit 9f14914.
The only change is a fix for the "register" iterator test to not rely on particular register names.
I mistook where the artificial "pc" register is generated. It isn't added to the register list or the register sets (except on arm where that's the name of the actual register), so I can't use it in this test. I instead just assert that the "register" generator produces the same list as flattening the register sets from "registers".
This reverts commit 9f14914.