Label callback uses and instruction cloning inconsistent. #3962
Labels
Comments
hgreving2304
pushed a commit
that referenced
this issue
Nov 26, 2019
Label callbacks are called every time an instruction is destroyed iff the instruction is a label instruction and the field is valid. When cloning an instruction, we can't blindly copy the label callback field causing it to be called when the cloned instruction is destroyed. We are adding a note to the instr_clone() docs and prevent the field from being copied. Fixes #3926 Issue: #3962
|
PR #3960 was broken: it didn't even test cloning. The scatter test doesn't have a loop around any emulation markers; adding one immediately hits the assert: |
derekbruening
added a commit
that referenced
this issue
Apr 29, 2021
PR #3960 added a call to instr_set_label_callback() to set it to NULL from instr_clone(), but if the callback is non-NULL an assert fires in that case. This only normally happens with emulation labels that turn into traces, which happens to not occur in our very few tests of emulation labels (#3173 covers adding more tests). We fix that by adding instr_clear_label_callback() here and using that from instr_clone(), since these callbacks are a little different from other values and it feels best to not clear them using the set routine. A test is added by putting a loop around a scatter-gather expansion, triggering trace creation. I confirmed that the assert does fire without this fix with the loop in place. Fixes #3962
derekbruening
added a commit
that referenced
this issue
Apr 29, 2021
PR #3960 added a call to instr_set_label_callback() to set it to NULL from instr_clone(), but if the callback is non-NULL an assert fires in that case. This only normally happens with emulation labels that turn into traces, which happens to not occur in our very few tests of emulation labels (#3173 covers adding more tests). We fix that by adding instr_clear_label_callback() here and using that from instr_clone(), since these callbacks are a little different from other values and it feels best to not clear them using the set routine. A test is added by putting a loop around a scatter-gather expansion, triggering trace creation. I confirmed that the assert does fire without this fix with the loop in place. Fixes #3962
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
For label instructions with allow the raw bytes field to be re-used for a label callback pointer. If the pointer is valid, it is called when the label instruction is destroyed. This was causing problems when the instruction is cloned, xref #3960.
We are fixing the hot immediate issue by explicitly not copying the label callback pointer. However, the deeper issue remains about the higher level semantics. Is the cloned instruction passed to the client and the pointer is now invalid? Do we need to make a deep copy? Or should it be transferred? Whatever the solution is, it needs to avoid having two instructions with the same valid pointer since the callback should be unique to an instruction and should only be called once when this instruction is destroyed.
The text was updated successfully, but these errors were encountered: