Skip to content

bpo-42246: Make sure that line number is correct after a return, as required by PEP 626 #23495

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
markshannon merged 4 commits into from
Dec 2, 2020
Merged

bpo-42246: Make sure that line number is correct after a return, as required by PEP 626 #23495

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
416779a
Make sure that line number is correct after a return, as defined by P…
markshannon Nov 23, 2020
33d3cb8
Add NEWS and update importlib.
markshannon Nov 24, 2020
b142622
Make sure implicit returns directly following for-loops have the corr…
markshannon Nov 25, 2020
548d343
Fix formatting and tidy up.
markshannon Dec 2, 2020
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
35 changes: 34 additions & 1 deletion Lib/test/test_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def test_leading_newlines(self):
s256 = "".join(["\n"] * 256 + ["spam"])
co = compile(s256, 'fn', 'exec')
self.assertEqual(co.co_firstlineno, 1)
self.assertEqual(list(co.co_lines()), [(0, 4, 257), (4, 8, None)])
self.assertEqual(list(co.co_lines()), [(0, 8, 257)])

def test_literals_with_leading_zeroes(self):
for arg in ["077787", "0xj", "0x.", "0e", "090000000000000",
Expand Down Expand Up @@ -775,6 +775,39 @@ def or_false(x):
self.assertIn('LOAD_', opcodes[0].opname)
self.assertEqual('RETURN_VALUE', opcodes[1].opname)

def test_lineno_after_implicit_return(self):
TRUE = True
# Don't use constant True or False, as compiler will remove test
def if1(x):
x()
if TRUE:
pass
def if2(x):
x()
if TRUE:
pass
else:
pass
def if3(x):
x()
if TRUE:
pass
else:
return None
def if4(x):
x()
if not TRUE:
pass
funcs = [ if1, if2, if3, if4]
lastlines = [ 3, 3, 3, 2]
frame = None
def save_caller_frame():
nonlocal frame
frame = sys._getframe(1)
for func, lastline in zip(funcs, lastlines, strict=True):
with self.subTest(func=func):
func(save_caller_frame)
self.assertEqual(frame.f_lineno-frame.f_code.co_firstlineno, lastline)

def test_big_dict_literal(self):
# The compiler has a flushing point in "compiler_dict" that calls compiles
Expand Down
12 changes: 8 additions & 4 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,14 @@ def bug708901():
12 STORE_FAST 0 (res)

%3d 14 JUMP_ABSOLUTE 10
>> 16 LOAD_CONST 0 (None)

%3d >> 16 LOAD_CONST 0 (None)
18 RETURN_VALUE
""" % (bug708901.__code__.co_firstlineno + 1,
bug708901.__code__.co_firstlineno + 2,
bug708901.__code__.co_firstlineno + 1,
bug708901.__code__.co_firstlineno + 3)
bug708901.__code__.co_firstlineno + 3,
bug708901.__code__.co_firstlineno + 1)


def bug1333982(x=[]):
Expand Down Expand Up @@ -295,13 +297,15 @@ def bug1333982(x=[]):
52 STORE_FAST 0 (e)
54 DELETE_FAST 0 (e)
56 RERAISE
>> 58 RERAISE

%3d >> 58 RERAISE
""" % (TRACEBACK_CODE.co_firstlineno + 1,
TRACEBACK_CODE.co_firstlineno + 2,
TRACEBACK_CODE.co_firstlineno + 5,
TRACEBACK_CODE.co_firstlineno + 3,
TRACEBACK_CODE.co_firstlineno + 4,
TRACEBACK_CODE.co_firstlineno + 5)
TRACEBACK_CODE.co_firstlineno + 5,
TRACEBACK_CODE.co_firstlineno + 3)

def _fstring(a, b, c, d):
return f'{a} {b:4} {c!r} {d!r:4}'
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2020-11-24-14-01-43.bpo-42246.c9k9hj.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
PEP 626: After a return, the f_lineno attribute of a frame is always the
last line executed.
144 changes: 134 additions & 10 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,28 @@ compiler_use_next_block(struct compiler *c, basicblock *block)
return block;
}

static basicblock *
compiler_copy_block(struct compiler *c, basicblock *block)
{
/* Cannot copy a block if it has a fallthrough, since
* a block can only have one fallthrough predecessor.
*/
assert(block->b_nofallthrough);
basicblock *result = compiler_next_block(c);
if (result == NULL) {
return NULL;
}
for(int i = 0; i < block->b_iused; i++) {

@pablogsal pablogsal Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
for(int i = 0; i < block->b_iused; i++) {
for (int i = 0; i < block->b_iused; i++) {

int n = compiler_next_instr(result);
if (n < 0) {
return NULL;
}
result->b_instr[n] = block->b_instr[i];
}
result->b_exit = block->b_exit;
return result;
}

/* Returns the offset of the next instruction in the current block's
b_instr array. Resizes the b_instr as necessary.
Returns -1 on failure.
Expand Down Expand Up @@ -2732,12 +2754,13 @@ compiler_if(struct compiler *c, stmt_ty s)
static int
compiler_for(struct compiler *c, stmt_ty s)
{
basicblock *start, *cleanup, *end;
basicblock *start, *body, *cleanup, *end;

start = compiler_new_block(c);
body = compiler_new_block(c);
cleanup = compiler_new_block(c);
end = compiler_new_block(c);
if (start == NULL || end == NULL || cleanup == NULL) {
if (start == NULL || body == NULL || end == NULL || cleanup == NULL) {
return 0;
}
if (!compiler_push_fblock(c, FOR_LOOP, start, end, NULL)) {
Expand All @@ -2747,6 +2770,7 @@ compiler_for(struct compiler *c, stmt_ty s)
ADDOP(c, GET_ITER);
compiler_use_next_block(c, start);
ADDOP_JUMP(c, FOR_ITER, cleanup);
compiler_use_next_block(c, body);

@pablogsal pablogsal Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this needed? To repeat the "body" if there is an implicit return? If so, could you add an example to the test?

@markshannon markshannon Dec 2, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FOR_ITER is a branch, so we need a new block immediately after it.
https://github.com/python/cpython/pull/23495/files#diff-ebc983d9f91e5bcf73500e377ac65e85863c4f77fd5b6b6caf4fcdf7c0f0b057R6401 checks it.

VISIT(c, expr, s->v.For.target);
VISIT_SEQ(c, stmt, s->v.For.body);
ADDOP_JUMP(c, JUMP_ABSOLUTE, start);
Expand Down Expand Up @@ -5929,9 +5953,16 @@ dump_basicblock(const basicblock *b)
}
#endif


static int
normalize_cfg(basicblock *bb);

static int
optimize_cfg(struct assembler *a, PyObject *consts);

static int
ensure_exits_have_lineno(struct compiler *c);

static PyCodeObject *
assemble(struct compiler *c, int addNone)
{
Expand All @@ -5952,6 +5983,14 @@ assemble(struct compiler *c, int addNone)
ADDOP(c, RETURN_VALUE);
}

if (normalize_cfg(c->u->u_blocks)) {
goto error;
}

if (ensure_exits_have_lineno(c)) {
goto error;
}

nblocks = 0;
entryblock = NULL;
for (b = c->u->u_blocks; b != NULL; b = b->b_list) {
Expand All @@ -5966,6 +6005,7 @@ assemble(struct compiler *c, int addNone)
else
c->u->u_firstlineno = 1;
}

if (!assemble_init(&a, nblocks, c->u->u_firstlineno))
goto error;
a.a_entry = entryblock;
Expand Down Expand Up @@ -6338,7 +6378,6 @@ clean_basic_block(basicblock *bb) {
bb->b_iused = dest;
}


static int
normalize_basic_block(basicblock *bb) {
/* Mark blocks as exit and/or nofallthrough.
Expand All @@ -6349,7 +6388,8 @@ normalize_basic_block(basicblock *bb) {
case RAISE_VARARGS:
case RERAISE:
bb->b_exit = 1;
/* fall through */
bb->b_nofallthrough = 1;
break;
case JUMP_ABSOLUTE:
case JUMP_FORWARD:
bb->b_nofallthrough = 1;
Expand All @@ -6358,16 +6398,32 @@ normalize_basic_block(basicblock *bb) {
case POP_JUMP_IF_TRUE:
case JUMP_IF_FALSE_OR_POP:
case JUMP_IF_TRUE_OR_POP:
case FOR_ITER:
if (i != bb->b_iused-1) {
PyErr_SetString(PyExc_SystemError, "malformed control flow graph.");
return -1;
}
/* Skip over empty basic blocks. */
while (bb->b_instr[i].i_target->b_iused == 0) {
bb->b_instr[i].i_target = bb->b_instr[i].i_target->b_next;
}

}
}
return 0;
}


static int

@pablogsal pablogsal Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
static int
static inline int

@markshannon markshannon Dec 2, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've deleted the function and inlined its body of the function. It was only used in once place.

normalize_cfg(basicblock *b) {
for (; b != NULL; b = b->b_list) {
if (normalize_basic_block(b)) {
return -1;
}
}
return 0;
}

static int
mark_reachable(struct assembler *a) {
basicblock **stack, **sp;
Expand Down Expand Up @@ -6398,8 +6454,27 @@ mark_reachable(struct assembler *a) {
return 0;
}

/* If an instruction has no line number, but it's predecessor in the BB does,
* then copy the line number. This reduces the size of the line number table,
* but has no impact on the generated line number events.
*/
static void
minimize_lineno_table(struct assembler *a) {
for (basicblock *b = a->a_entry; b != NULL; b = b->b_next) {
int prev_lineno = -1;
for (int i = 0; i < b->b_iused; i++) {
if (b->b_instr[i].i_lineno < 0) {
b->b_instr[i].i_lineno = prev_lineno;
}
else {
prev_lineno = b->b_instr[i].i_lineno;
}
}

/* Perform basic peephole optimizations on a control flow graph.
}
}

/* Perform optimizations on a control flow graph.
The consts object should still be in list form to allow new constants
to be appended.

Expand All @@ -6411,11 +6486,6 @@ mark_reachable(struct assembler *a) {
static int
optimize_cfg(struct assembler *a, PyObject *consts)
{
for (basicblock *b = a->a_entry; b != NULL; b = b->b_next) {
if (normalize_basic_block(b)) {
return -1;
}
}
for (basicblock *b = a->a_entry; b != NULL; b = b->b_next) {
if (optimize_basic_block(b, consts)) {
return -1;
Expand All @@ -6432,9 +6502,63 @@ optimize_cfg(struct assembler *a, PyObject *consts)
b->b_iused = 0;
}
}
minimize_lineno_table(a);
return 0;
}

static int
is_exit_without_lineno(basicblock *b) {
return b->b_exit && b->b_instr[0].i_lineno < 0;
}

@pablogsal pablogsal Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
static int
is_exit_without_lineno(basicblock *b) {
return b->b_exit && b->b_instr[0].i_lineno < 0;
}
static inline int
is_exit_without_lineno(basicblock *b) {
return b->b_exit && b->b_instr[0].i_lineno < 0;
}


/* PEP 626 mandates that the f_lineno of a frame is correct
* after a frame terminates. It would be prohibitively expensive
* to continuously update the f_lineno field at runtime,
* so we make sure that all exiting instruction (raises and returns)
* have a valid line number, allowing us to compute f_lineno lazily.
* We can do this by duplicating the exit blocks without line number
* so that none have more than one predecessor. We can then safely
* copy the line number from the sole predecessor block.
*/
static int
ensure_exits_have_lineno(struct compiler *c)
{
/* Copy all exit blocks without line number that are targets of a jump.
*/
for (basicblock *b = c->u->u_blocks; b != NULL; b = b->b_list) {
if (b->b_iused > 0 && is_jump(&b->b_instr[b->b_iused-1])) {
switch(b->b_instr[b->b_iused-1].i_opcode) {

@pablogsal pablogsal Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
switch(b->b_instr[b->b_iused-1].i_opcode) {
switch (b->b_instr[b->b_iused-1].i_opcode) {

/* Note: Only actual jumps, not exception handlers */
case SETUP_ASYNC_WITH:
case SETUP_WITH:
case SETUP_FINALLY:
continue;
}
basicblock *target = b->b_instr[b->b_iused-1].i_target;
if (is_exit_without_lineno(target)) {
basicblock *new_target = compiler_copy_block(c, target);
if (new_target == NULL) {
return -1;
}
new_target->b_instr[0].i_lineno = b->b_instr[b->b_iused-1].i_lineno;
b->b_instr[b->b_iused-1].i_target = new_target;
}
}
}
/* Any remaining reachable exit blocks without line number can only be reached by
* fall through, and thus can only have a single predecessor */
for (basicblock *b = c->u->u_blocks; b != NULL; b = b->b_list) {
if (!b->b_nofallthrough && b->b_next && b->b_iused > 0) {
if (is_exit_without_lineno(b->b_next)) {
assert(b->b_next->b_iused > 0);
b->b_next->b_instr[0].i_lineno = b->b_instr[b->b_iused-1].i_lineno;
}
}
}
return 0;
}


/* Retained for API compatibility.
* Optimization is now done in optimize_cfg */

Expand Down
Loading