Repository navigation
GH-123044: Give the POP_TOP after a case test a location in the body, not the pattern. - #130627
markshannon merged 10 commits into
Conversation
…cations for BRANCH events.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
… next instruction
| .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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
…d propagate_line_numbers accordingly.
|
The fuzzer found an example where the case body consists solely of a local variable annotation: ...
case _:
l: int
Since local variable annotations are removed from the compiler, there is no following instruction in the case body. |
Sorry, something went wrong.
…tion to use. Assert NEXT_LOCATION never gets emitted
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.