From 41cfadcd6f59132e76e87793d516fbb8afa32160 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 9 Jun 2022 15:22:48 -0700 Subject: [PATCH 1/6] Update tests for intended behavior --- Lib/test/test_dis.py | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_dis.py b/Lib/test/test_dis.py index a94cd6432e1232..0f8e8b3a66f5a8 100644 --- a/Lib/test/test_dis.py +++ b/Lib/test/test_dis.py @@ -1046,8 +1046,10 @@ def test_show_caches(self): caches = list(self.get_cached_values(quickened, adaptive)) for cache in caches: self.assertRegex(cache, pattern) - self.assertEqual(caches.count(""), 8) - self.assertEqual(len(caches), 22) + total_caches = 22 + empty_caches = 8 if adaptive and quickened else total_caches + self.assertEqual(caches.count(""), empty_caches) + self.assertEqual(len(caches), total_caches) class DisWithFileTests(DisTests): @@ -1599,7 +1601,33 @@ def test_co_positions_missing_info(self): self.assertIsNone(positions.end_lineno) self.assertIsNone(positions.col_offset) self.assertIsNone(positions.end_col_offset) - + + @requires_debug_ranges() + def test_co_positions_with_lots_of_caches(self): + def roots(a, b, c): + d = (b**2 - 4 * a * c) ** 0.5 + yield (-b - d) / (2 * a) + if d: + yield (-b + d) / (2 * a) + code = roots.__code__ + for show_caches in (False, True): + for adaptive in (False, True): + with self.subTest(f"{adaptive=}, {show_caches=}"): + co_positions = [ + positions + for op, positions in zip( + code.co_code[::2], code.co_positions(), strict=True + ) + if show_caches or op != opcode.opmap["CACHE"] + ] + dis_positions = [ + instruction.positions + for instruction in dis.get_instructions( + code, adaptive=adaptive, show_caches=show_caches + ) + ] + self.assertEqual(co_positions, dis_positions) + # get_instructions has its own tests above, so can rely on it to validate # the object oriented API class BytecodeTests(InstructionTestCase, DisTestBase): From 0653bc5684c62b7e9fe68d5209a93411d93705cd Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 9 Jun 2022 15:23:57 -0700 Subject: [PATCH 2/6] Fix interactions between Positions and CACHEs --- Lib/dis.py | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/Lib/dis.py b/Lib/dis.py index 5a5ee8d848d3a4..5ab830af54c621 100644 --- a/Lib/dis.py +++ b/Lib/dis.py @@ -492,17 +492,29 @@ def _get_instructions_bytes(code, varname_from_oparg=None, yield Instruction(_all_opname[op], op, arg, argval, argrepr, offset, starts_line, is_jump_target, positions) - if show_caches and _inline_cache_entries[deop]: - for name, caches in _cache_format[opname[deop]].items(): - data = code[offset + 2: offset + 2 + caches * 2] - argrepr = f"{name}: {int.from_bytes(data, sys.byteorder)}" - for _ in range(caches): - offset += 2 - yield Instruction( - "CACHE", 0, 0, None, argrepr, offset, None, False, None - ) - # Only show the actual value for the first cache entry: + caches = _inline_cache_entries[deop] + if not caches: + continue + if not show_caches: + # We still need to advance the co_positions iterator: + for _ in range(caches): + next(co_positions, ()) + continue + for name, size in _cache_format[opname[deop]].items(): + for i in range(size): + offset += 2 + # Only show the fancy argrepr for a CACHE instruction when it's + # the first entry for a particular cache value and the + # instruction using it is actually quickened: + if i == 0 and op != deop: + data = code[offset: offset + 2 * size] + argrepr = f"{name}: {int.from_bytes(data, sys.byteorder)}" + else: argrepr = "" + yield Instruction( + "CACHE", CACHE, 0, None, argrepr, offset, None, False, + Positions(*next(co_positions, ())) + ) def disassemble(co, lasti=-1, *, file=None, show_caches=False, adaptive=False): """Disassemble a code object.""" From 44c742aaad2fc0fd46ddf6cf189eb06d882b1802 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 9 Jun 2022 17:16:03 -0700 Subject: [PATCH 3/6] blurb add --- .../next/Library/2022-06-09-17-15-26.gh-issue-91389.OE4vS5.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-06-09-17-15-26.gh-issue-91389.OE4vS5.rst diff --git a/Misc/NEWS.d/next/Library/2022-06-09-17-15-26.gh-issue-91389.OE4vS5.rst b/Misc/NEWS.d/next/Library/2022-06-09-17-15-26.gh-issue-91389.OE4vS5.rst new file mode 100644 index 00000000000000..0a126551e4110b --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-06-09-17-15-26.gh-issue-91389.OE4vS5.rst @@ -0,0 +1,2 @@ +Fix an issue where :mod:`dis` utilities could report missing or incorrect +position information in the presence of ``CACHE`` entries. From 986ce3a1ad0020a147145eb0241d5d078b3586df Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 9 Jun 2022 21:00:36 -0700 Subject: [PATCH 4/6] make patchcheck --- Lib/test/test_dis.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_dis.py b/Lib/test/test_dis.py index 0f8e8b3a66f5a8..1bcaf9d09a1593 100644 --- a/Lib/test/test_dis.py +++ b/Lib/test/test_dis.py @@ -1601,7 +1601,7 @@ def test_co_positions_missing_info(self): self.assertIsNone(positions.end_lineno) self.assertIsNone(positions.col_offset) self.assertIsNone(positions.end_col_offset) - + @requires_debug_ranges() def test_co_positions_with_lots_of_caches(self): def roots(a, b, c): @@ -1627,7 +1627,7 @@ def roots(a, b, c): ) ] self.assertEqual(co_positions, dis_positions) - + # get_instructions has its own tests above, so can rely on it to validate # the object oriented API class BytecodeTests(InstructionTestCase, DisTestBase): From 8266ba9c658609a25cbe15dd66bc63b4f58878f1 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 15 Jun 2022 17:40:26 -0700 Subject: [PATCH 5/6] Feedback from code review --- Lib/test/test_dis.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_dis.py b/Lib/test/test_dis.py index d5bd6dedd3c05c..d8c06d2c635709 100644 --- a/Lib/test/test_dis.py +++ b/Lib/test/test_dis.py @@ -1756,20 +1756,24 @@ def test_co_positions_missing_info(self): @requires_debug_ranges() def test_co_positions_with_lots_of_caches(self): def roots(a, b, c): - d = (b**2 - 4 * a * c) ** 0.5 - yield (-b - d) / (2 * a) + d = b**2 - 4 * a * c + yield (-b - cmath.sqrt(d)) / (2 * a) if d: - yield (-b + d) / (2 * a) + yield (-b + cmath.sqrt(d)) / (2 * a) code = roots.__code__ + ops = code.co_code[::2] + cache = opcode.opmap["CACHE"] + caches = sum(op == cache for op in ops) + non_caches = len(ops) - caches + # Make sure we have "lots of caches". If not, roots should be updated: + assert 1 / 3 <= caches / non_caches, "this test needs more caches!" for show_caches in (False, True): for adaptive in (False, True): with self.subTest(f"{adaptive=}, {show_caches=}"): co_positions = [ positions - for op, positions in zip( - code.co_code[::2], code.co_positions(), strict=True - ) - if show_caches or op != opcode.opmap["CACHE"] + for op, positions in zip(ops, code.co_positions(), strict=True) + if show_caches or op != cache ] dis_positions = [ instruction.positions From d500bf2a0a7875618889919d3a979db0e0e8b33e Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 16 Jun 2022 12:03:46 -0700 Subject: [PATCH 6/6] Naming is hard --- Lib/test/test_dis.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_dis.py b/Lib/test/test_dis.py index d8c06d2c635709..a9b43a82496bcd 100644 --- a/Lib/test/test_dis.py +++ b/Lib/test/test_dis.py @@ -1762,10 +1762,10 @@ def roots(a, b, c): yield (-b + cmath.sqrt(d)) / (2 * a) code = roots.__code__ ops = code.co_code[::2] - cache = opcode.opmap["CACHE"] - caches = sum(op == cache for op in ops) + cache_opcode = opcode.opmap["CACHE"] + caches = sum(op == cache_opcode for op in ops) non_caches = len(ops) - caches - # Make sure we have "lots of caches". If not, roots should be updated: + # Make sure we have "lots of caches". If not, roots should be changed: assert 1 / 3 <= caches / non_caches, "this test needs more caches!" for show_caches in (False, True): for adaptive in (False, True): @@ -1773,7 +1773,7 @@ def roots(a, b, c): co_positions = [ positions for op, positions in zip(ops, code.co_positions(), strict=True) - if show_caches or op != cache + if show_caches or op != cache_opcode ] dis_positions = [ instruction.positions