GH-123044: Give the POP_TOP after a case test a location in the body, not the pattern.
#130627
Conversation
…cations for BRANCH events.
markshannon
changed the title
POP_TOP after a case test a location in the body, not the pattern.POP_TOP after a case test a location in the body, not the pattern.
Python/codegen.c
Outdated
| ADDOP(c, LOC(m->pattern), POP_TOP); | ||
| /* Use the body location to give better locations for branch events */ | ||
| assert(asdl_seq_LEN(m->body) > 0); | ||
| ADDOP(c, LOC(asdl_seq_GET(m->body, 0)), POP_TOP); |
There was a problem hiding this comment.
Shouldn't this be the location of the last instruction in the block? Line tracing might look like we executed that line more than once if we "return" to it after running another line.
There was a problem hiding this comment.
I've added a NEXT_LOCATION pseudo location so that the POP_TOP gets the location of whatever follows it.
… next instruction
Include/internal/pycore_symtable.h
Outdated
| .end_col_offset = (n)->end_col_offset } | ||
|
|
||
| static const _Py_SourceLocation NO_LOCATION = {-1, -1, -1, -1}; | ||
| static const _Py_SourceLocation NEXT_LOCATION = {INT_MAX, INT_MAX, INT_MAX, INT_MAX}; |
There was a problem hiding this comment.
This is a good idea.
We probably need to change remove_redundant_nops to treat this as no location. Maybe it should be (-2, -2, -2, -2) and then just compare to 0?
There was a problem hiding this comment.
There are some checks for NO_LOCATION that compare to 0, but it isn't clear to me which should support NEXT_LOCATION and which shouldn't, so I'm a bit reluctant to do that.
There was a problem hiding this comment.
With {2, -2, -2, -2}, basicblock_remove_redundant_nops and propagate_line_numbers needed changing.
basicblock_remove_redundant_nops needs to treat NEXT_LOCATION like NO_LOCATION
propagate_line_numbers needs to treat NEXT_LOCATION not like NO_LOCATION
| if (same_location(instr[-1].i_loc, NEXT_LOCATION)) { | ||
| instr[-1].i_loc = instr->i_loc; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this transformation should be in propagate_line_numbers in flowgraph.c. Otherwise the last instruction may remain without location, when it could have received a location from an earlier instruction in its block.
There was a problem hiding this comment.
Wouldn't it be too early in propagate_line_numbers?
If the NEXT_LOCATION instruction is at the end of the block where would it get the location information from?
There was a problem hiding this comment.
From the next block (fall through or jump). If there is more than one successor, then we have a problem anyway, right, so what do we do?
There was a problem hiding this comment.
We should only use NEXT_LOCATION if instruction is not the last instruction in the block. I'll add an assert for that.
…d propagate_line_numbers accordingly.
|
The fuzzer found an example where the case body consists solely of a local variable annotation: Since local variable annotations are removed from the compiler, there is no following instruction in the case body. |
…tion to use. Assert NEXT_LOCATION never gets emitted
markshannon
force-pushed
the
location-pop-top-after-case
branch
from
859aa5a to
a077433
Compare
…he body, not the pattern. (pythonGH-130627)
2 participants
matchcases showing incorrectly #123044