Skip to content

py/objcode: Implement PEP626 to add co_lines to code objects on settrace builds. #16989

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.

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

AJMansfield
Copy link
Contributor

Summary

This PR implements PEP 626 – Precise line numbers for debugging and other tools..

This adds one method, co_lines, that returns an iterator over (begin, end, line_no) tuples of line numbers for applicable bytecode ranges.

Testing

This PR includes automated tests of all of the currently-implemented code object attributes, including the existing attributes that previously had no test coverage.

The actual values in co_lines can't be compared directly to CPython's, but all of the invariants that the method is meant to observe are checked.

In addition to the github workflow checks, I've also manually verified that all tests pass when run against a unix MICROPY_PY_SYS_SETTRACE build. (In particular, I've checked to make sure they're not skipped).

Trade-offs and Alternatives

As in CPython, this is implemented as its own distinct iterator object. Potentially, this function could be functionally replicated by instead eagerly building a list of the tuples and returning an iterator over that list, at a slightly smaller code size; but the iterator approach allows for a tighter behavioral match with CPython (fun_code_full.py:25), and code size is not at nearly the same premium in settrace builds.

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.56%. Comparing base (f1018ee) to head (45623a4).
Report is 441 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16989      +/-   ##
==========================================
+ Coverage   98.54%   98.56%   +0.02%     
==========================================
  Files         169      169              
  Lines       21877    21945      +68     
==========================================
+ Hits        21558    21631      +73     
+ Misses        319      314       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

github-actions bot commented Mar 21, 2025

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +8 +0.001% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@AJMansfield AJMansfield changed the title py/objcode: Implement PEP626 to add the co_lines to code objects. py/objcode: Implement PEP626 to add co_lines to code objects on settrace builds. Mar 21, 2025
@Gadgetoid
Copy link
Contributor

Related to #16797?

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Mar 27, 2025
Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like a nice addition!

I think we should then deprecate co_lnotab following CPython.

And it would be good to add another test to this PR, to test the actual numeric values from co_lines(), to make sure they match the expected bytecode offsets for a simple test function (that test would need a corresponding .py.exp file because you can't use CPython to work out the right answer).

@AJMansfield
Copy link
Contributor Author

Hmm, actually, looking at the compilation result, it looks like gcc -O3 isn't quite strong enough to make the whole result.info_increment part fully transparent --- at least in ARM, there's two extra opcodes with it, associated with moving around that 1 or 2 increment value and incrementing the line_info value with it.

https://godbolt.org/z/hqoYodEaE

@AJMansfield
Copy link
Contributor Author

AJMansfield commented Jun 26, 2025

There we are, having it return the new line_info value instead of an increment value for it manages to get it fully elided: https://godbolt.org/z/bEY6ffTK8

Though, that makes it much less clear what's going on --- and there's similar differences in the other contexts with how incrementing bc and the like gets rolled in; doing the same thing for them makes it even harder and also ends up pessimizing the mp_bytecode_get_source_line to something even larger than without it.

(Also, clang gets the exact same compilation result regardless of whether or which way I refactor it.)

I'll leave the PR as-is unless we really feel like it's important to turn the screws as tight as possible just to win back 2 opcodes here.

@AJMansfield
Copy link
Contributor Author

AJMansfield commented Jun 26, 2025

The thread/thread_gc1.py test failure in unix port / settrace_stackless (pull_request) seems to be spurious.

Was initially concerned if there was some sort of overflow making mp_obj_malloc(mp_obj_colines_iter_t, &mp_type_polymorph_iter); somehow a leak (with the fact that mp_obj_colines_iter_t is a lot larger than the mp_obj_iter_buf_t type used for this in mp_obj_tuple_getiter) --- but that test doesn't even interact with code objects, so idk.

@AJMansfield AJMansfield force-pushed the code_co_lines branch 4 times, most recently from 5c31dc4 to c27a6ad Compare July 1, 2025 17:38
@AJMansfield
Copy link
Contributor Author

AJMansfield commented Jul 1, 2025

And it would be good to add another test to this PR, to test the actual numeric values from co_lines(), to make sure they match the expected bytecode offsets for a simple test function (that test would need a corresponding .py.exp file because you can't use CPython to work out the right answer).

I've been playing around with these tests; at the moment the test I added also covers the cases where a correct co_lines implementation might need to sum together multiple successive table entries to get the correct answer --- which it currently doesn't do.

The current outputs from co_lines are still meaningful and correct in some sense, though --- is that summing behavior actually something this needs to implement? Or can these extra coverage cases just be dropped (or made into cpydiffs or something)?

Also, should I add coverage for a case where the table needs to skip more than 2048 lines at once? (e.g. to overflow the 11-bit line number increment in the 2-byte format, and require a multi-entry encoding for it like bc+0, line+2047; bc+4, line+12)

@AJMansfield
Copy link
Contributor Author

AJMansfield commented Jul 1, 2025

Ok, in the most recent variant of the test I've tried, I'm now really confused, and suspect I may have found a bug with the emitter for the underlying line info tables.

The function in question, which I've annotated with what seems to be the line-number encodings, is:

def f(x, y, obj, obj2, obj3):
    a = x + y # line 4: bc+4 line+4
    b = x - y # line 5: bc+4 line+1
    # line 6
    # line 7
    # line 8
    # line 9
    # line 10
    # line 11
    # line 12
    # line 13
    # line 14
    # line 15
    # line 16
    # line 17
    # line 18
    # line 19
    c = a * b # line 20: bc+4 line+15
    obj.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.fun() # line 21: bc+31 line+1; bc+27 line+0
    # line 22
    # line 23
    # line 24: bc+0 line+3
    # line 25
    # line 26
    # line 27: bc+0 line+3
    # line 28
    # line 29
    obj2.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.fun() # line 30: bc+31 line+3; bc+27 line+0
    # line 31
    # line 32
    # line 33: bc+0 line+3
    # line 34
    # line 35
    # line 36
    # line 37
    # line 38
    # line 39
    # line 40
    # line 41
    # line 42
    # line 43
    # line 44
    # line 45
    # line 46
    # line 47
    # line 48
    # line 49
    # line 50
    # line 51
    # line 52
    # line 53
    # line 54
    # line 55
    # line 56
    # line 57
    # line 58
    # line 59
    obj3.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.fun() # line 60: bc+31 line+27; bc+27 line+0
    return c # line 61: bc+2 line+1

I'd expect line 60 to get encoded as bc+15 line+27; bc+31 line+0; bc+11 line+0, but it seems to end up getting encoded in a much weirder way, with that bc+0 line+3 for line 33 and then bc+31 line+27 to jump from there to line 60 --- which doesn't seem like it should be a valid encoding in either the 1-byte or 2-byte formats.

I've not dug in to see if this is related to the SyntaxError this causes in the unix minimal and zephyr ports, but it doesn't seem far-fetched that this might also be part of that.

@dpgeorge
Copy link
Member

dpgeorge commented Jul 2, 2025

I'd expect line 60 to get encoded as bc+15 line+27; bc+31 line+0; bc+11 line+0, but it seems to end up getting encoded in a much weirder way, with that bc+0 line+3 for line 33 and then bc+31 line+27 to jump from there to line 60 --- which doesn't seem like it should be a valid encoding in either the 1-byte or 2-byte formats.

It's correct.

The line number encoding for f is:

60:24:84:0f:24:1f:7b:60:60:1f:7b:80:1b:1f:3b

And that decodes to the following (b,l) pairs:

60    = 0,3
24    = 4,1
80:0f = 4,15
24    = 4,1
1f    = 31,0
7b    = 27,3
60    = 0,3
60    = 0,3
1f    = 31,0
7b    = 27,3
80:1b = 0,27
1f    = 31,0
3b    = 27,1

@dpgeorge
Copy link
Member

dpgeorge commented Jul 2, 2025

I've been playing around with these tests; at the moment the test I added also covers the cases where a correct co_lines implementation might need to sum together multiple successive table entries to get the correct answer --- which it currently doesn't do.

I'm not sure what you mean by "sum together multiple successive table entries"?

Also, should I add coverage for a case where the table needs to skip more than 2048 lines at once?

No, I think it's fine without that case.

@AJMansfield
Copy link
Contributor Author

AJMansfield commented Jul 3, 2025

I'm not sure what you mean by "sum together multiple successive table entries"?

Currently, running the fun_code_colines test from c27a6ad on a build with the appropriate features enabled for it results in a test failure --- in that, the corresponding .exp file I created for it includes only information about the actual endpoints of the bytecode ranges for the function's non-empty lines.

However, the output of co_lines as currently implemented in 19bc295, produces (a) extra 'spurious' entries corresponding to the bc+0 info bytes sometimes used to skip the line number ahead when reaching numeric limits, and (b) strings of split entries corresponding to the line+0 info bytes needed to attribute more than 31 instructions to a single line:

 line 4 start: 0
 line 4 end: 4
 line 5 start: 4
 line 5 end: 8
 line 20 start: 8
 line 20 end: 12
 line 21 start: 12
+line 21 end: 43
+line 21 start: 43
 line 21 end: 70
+line 24 start: 70
+line 24 end: 70
+line 27 start: 70
+line 27 end: 70
 line 30 start: 70
+line 30 end: 101
+line 30 start: 101
 line 30 end: 128
+line 33 start: 128
+line 33 end: 128
 line 60 start: 128
+line 60 end: 159
+line 60 start: 159
 line 60 end: 186
 line 61 start: 186
 line 61 end: 188

There's a sense in which this output is still correct --- in that it describes the same mapping, and it's not difficult for an automated tool to accommodate this quirk. However, it would also be pretty easy to modify co_lines to just not emit these: to make it skip past bc+0 entries, and to combine the ranges of any line+0 entries following the current one.

@AJMansfield
Copy link
Contributor Author

AJMansfield commented Jul 3, 2025

I've done some digging to see what the spec says and what CPython does here:

It seems like the spurious bc+0 entries are actually totally allowed per PEP626 / zero-width-ranges:

Zero width range, that is ranges where start == end are allowed. Zero width ranges are used for lines that are present in the source code, but have been eliminated by the bytecode compiler.

The rest of PEP626 / the-new-co-lines-method-of-code-objects also requires that the bytecode ranges be contiguous and in order; and that does imply that line numbers would need to therefore not be unique or in order in an implementation that does instruction re-ordering. So, on a technical level, I think the implementation does actually meet the letter of the specification.


CPython's current implementation specifically checks for and combines multiple entries together in the code it uses to parse its line-number table --- but this is actually a difference at least between the Python 3.12 and 3.10 versions I checked to confirm.

Running that same test case file on CPython, but with the obj3.... line in the function replaced with one that's now thousands of bytecode instructions and needs an increment of thousands of lines to reach (to make sure it exceeds the single-entry numeric limits of CPython's table):

def f(x, y, obj, obj2, obj3):
    ...
    # ... all of the blank lines skipped ...
    # line 15283
    obj3().a[obj3().a0().a1().a2().a3().a4().a5().a6().a7().a8().a9]().b[obj3().b0().b1().b2().b3().b4().b5().b6().b7().b8().b9]().c[obj3().c0().c1().c2().c3().c4().c5().c6().c7().c8().c9]().d[obj3().d0().d1().d2().d3().d4().d5().d6().d7().d8().d9]().e[obj3().e0().e1().e2().e3().e4().e5().e6().e7().e8().e9]().f[obj3().f0().f1().f2().f3().f4().f5().f6().f7().f8().f9]().g[obj3().g0().g1().g2().g3().g4().g5().g6().g7().g8().g9]().h[obj3().h0().h1().h2().h3().h4().h5().h6().h7().h8().h9]().i[obj3().i0().i1().i2().i3().i4().i5().i6().i7().i8().i9]().j[obj3().j0().j1().j2().j3().j4().j5().j6().j7().j8().j9]().k[obj3().k0().k1().k2().k3().k4().k5().k6().k7().k8().k9]().l[obj3().l0().l1().l2().l3().l4().l5().l6().l7().l8().l9]().m[obj3().m0().m1().m2().m3().m4().m5().m6().m7().m8().m9]().n[obj3().n0().n1().n2().n3().n4().n5().n6().n7().n8().n9]().o[obj3().o0().o1().o2().o3().o4().o5().o6().o7().o8().o9]().p[obj3().p0().p1().p2().p3().p4().p5().p6().p7().p8().p9]().q[obj3().q0().q1().q2().q3().q4().q5().q6().q7().q8().q9]().r[obj3().r0().r1().r2().r3().r4().r5().r6().r7().r8().r9]().s[obj3().s0().s1().s2().s3().s4().s5().s6().s7().s8().s9]().t[obj3().t0().t1().t2().t3().t4().t5().t6().t7().t8().t9]().u[obj3().u0().u1().u2().u3().u4().u5().u6().u7().u8().u9]().v[obj3().v0().v1().v2().v3().v4().v5().v6().v7().v8().v9]().w[obj3().w0().w1().w2().w3().w4().w5().w6().w7().w8().w9]().x[obj3().x0().x1().x2().x3().x4().x5().x6().x7().x8().x9]().y[obj3().y0().y1().y2().y3().y4().y5().y6().y7().y8().y9]().z[obj3().z0().z1().z2().z3().z4().z5().z6().z7().z8().z9]().fun() # line 15284
    return c # line 15285

On Python 3.10.18, this produces multiple entries:

line 4 start: 0
line 4 end: 8
line 5 start: 8
line 5 end: 16
line 20 start: 16
line 20 end: 24
line 21 start: 24
line 21 end: 84
line 30 start: 84
line 30 end: 144
line 15284 start: 144
line 15284 end: 398
line 15284 start: 398
line 15284 end: 652
line 15284 start: 652
line 15284 end: 906
line 15284 start: 906
line 15284 end: 1160
line 15284 start: 1160
line 15284 end: 1414
line 15284 start: 1414
line 15284 end: 1464
line 15285 start: 1464
line 15285 end: 1468

But on Python 3.12.3 it produces only a single entry:

line 3 start: 0
line 3 end: 2
line 4 start: 2
line 4 end: 12
line 5 start: 12
line 5 end: 22
line 20 start: 22
line 20 end: 32
line 21 start: 32
line 21 end: 584
line 30 start: 584
line 30 end: 1136
line 15284 start: 1136
line 15284 end: 9764
line 15285 start: 9764
line 15285 end: 9768

Key Question: Should this feature implement logic to (a) skip lineinfo entries for blank lines, and (b) combine multiple lineinfo entries for a single line together, like newer (but not older) versions of CPython do? Or should I modify the test to be invariant to this detail (in the way that tools that consume co_lines are "supposed to" be)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants