Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
GH-123044: Give thePOP_TOP after a case test a location in the body, not the pattern.#130627
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
GH-123044: Give thePOP_TOP after a case test a location in the body, not the pattern.#130627
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…cations for BRANCH events.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've added aNEXT_LOCATION pseudo location so that thePOP_TOP gets the location of whatever follows it.
Include/internal/pycore_symtable.h Outdated
| .end_col_offset = (n)->end_col_offset } | ||
| staticconst_Py_SourceLocationNO_LOCATION= {-1,-1,-1,-1}; | ||
| staticconst_Py_SourceLocationNEXT_LOCATION= {INT_MAX,INT_MAX,INT_MAX,INT_MAX}; |
iritkatrielMar 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are some checks forNO_LOCATION that compare to 0, but it isn't clear to me which should supportNEXT_LOCATION and which shouldn't, so I'm a bit reluctant to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With {2, -2, -2, -2},basicblock_remove_redundant_nops andpropagate_line_numbers needed changing.basicblock_remove_redundant_nops needs to treat NEXT_LOCATION like NO_LOCATIONpropagate_line_numbers needs to treat NEXT_LOCATIONnot 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this transformation should be inpropagate_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wouldn't it be too early inpropagate_line_numbers?
If theNEXT_LOCATION instruction is at the end of the block where would it get the location information from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should only useNEXT_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
859aa5a toa077433Comparebe046ee intopython:mainUh oh!
There was an error while loading.Please reload this page.
…he body, not the pattern. (pythonGH-130627)
Uh oh!
There was an error while loading.Please reload this page.
matchcases showing incorrectly #123044