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.

@@            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.

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

Related to #16797?

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

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.

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.

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)

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.

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

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.

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.

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)?