bpo-42246: Make sure that line number is correct after a return, as required by PEP 626 - #23495
Conversation
Sorry, something went wrong.
gpshead
left a comment
There was a problem hiding this comment.
LGTM though it'd be a good idea to have more eyeballs than just mine on this. 👍
Sorry, something went wrong.
|
@pablogsal would you mind taking a quick look at this? |
Sorry, something went wrong.
Yep, will review this today |
Sorry, something went wrong.
| static int | ||
| is_exit_without_lineno(basicblock *b) { | ||
| return b->b_exit && b->b_instr[0].i_lineno < 0; | ||
| } |
There was a problem hiding this comment.
| 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; | |
| } |
Sorry, something went wrong.
| if (result == NULL) { | ||
| return NULL; | ||
| } | ||
| for(int i = 0; i < block->b_iused; i++) { |
There was a problem hiding this comment.
| for(int i = 0; i < block->b_iused; i++) { | |
| for (int i = 0; i < block->b_iused; i++) { |
Sorry, something went wrong.
| } | ||
|
|
||
|
|
||
| static int |
There was a problem hiding this comment.
| static int | |
| static inline int |
Sorry, something went wrong.
There was a problem hiding this comment.
I've deleted the function and inlined its body of the function. It was only used in once place.
Sorry, something went wrong.
| ADDOP(c, GET_ITER); | ||
| compiler_use_next_block(c, start); | ||
| ADDOP_JUMP(c, FOR_ITER, cleanup); | ||
| compiler_use_next_block(c, body); |
There was a problem hiding this comment.
Why is this needed? To repeat the "body" if there is an implicit return? If so, could you add an example to the test?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| */ | ||
| 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) { |
There was a problem hiding this comment.
| switch(b->b_instr[b->b_iused-1].i_opcode) { | |
| switch (b->b_instr[b->b_iused-1].i_opcode) { |
Sorry, something went wrong.
|
LGTM in general but I left some minor suggestions |
Sorry, something went wrong.
…equired by PEP 626 (pythonGH-23495) Make sure that line number is correct after a return, as defined by PEP 626.
This PR ensures that f_lineno can be correctly computed after a return, by ensuring that all compiler generated return instructions
have a line number.
Any block that has no line numbers and has only one predecessor can inherit the line number from its predecessor.
Any block ending in a return has no successors, and thus can be safely duplicated.
By duplicating all blocks that end in a return, but have no line number, those blocks will have only one predecessor and thus can inherit its predecessor's line number, meaning that all implicit returns now have a line number.
For example,
currently compiles to:
2 0 LOAD_FAST 0 (cond) 2 POP_JUMP_IF_FALSE 8 3 4 LOAD_GLOBAL 0 (a) 6 POP_TOP None >> 8 LOAD_CONST 0 (None) 10 RETURN_VALUEwith this PR it compiles to:
2 0 LOAD_FAST 0 (cond) 2 POP_JUMP_IF_FALSE 12 3 4 LOAD_GLOBAL 0 (a) 6 POP_TOP 8 LOAD_CONST 0 (None) 10 RETURN_VALUE 2 >> 12 LOAD_CONST 0 (None) 14 RETURN_VALUEwhich is equivalent, but produces the correct line number when not cond.
https://bugs.python.org/issue42246