Conversation
The previous branch hint instrumentation logic would introduce a scratch local to hold the condition so it could be passed into both the logging function and the original branching instruction. The de-instrumentation pass would then need to find this local and attempt to undo the data flow change. Simplify all of this by having the logging function return the condition value so it can interpose between the condition and the branch without any new locals. De-instrumentation can now just replace the call to the log function with its condition parameter. To allow further simplification, also change the order of parameters to the logging function so the condition value is the first parameter. This ensures that we don't need to introduce a scratch local even when the condition is a `pop`, because the pop will remain the leftmost leaf expression in the catch body.
tlively
force-pushed
the
branch-hint-transparent-instrument
branch
from
4356c25 to
1456ddf
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Nice!
Have you verified this catches bugs properly? E.g. removing the branch hint updates from RemoveUnusedBrs or OptimizeInstructions should quickly lead the fuzzer to a problem.
|
Yes, just confirmed that now. |
|
I see branch hint fuzzer errors after this landed, e.g. seed |
|
Taking a look. |
|
What commit does that seed come from? It doesn't reproduce on the current head or on the commit where this PR landed. Regardless, I'll run the fuzzer locally to see if I find problems. |
|
Weird, it works for me on both those commits. I guess there is not enough determinism in these seeds... I get an error every few thousand iterations, so hopefully you're reproduced by now. If not I can reduce one of them. |
|
Disabling all other fuzzers, I see a problem every 100 iterations or so. |
|
Ok, I have a reproducer. It looks like another bad interaction between the recent parser changes and this pass. Thankfully it shouldn't be too hard to fix. |
2 participants
The previous branch hint instrumentation logic would introduce a scratch local to hold the condition so it could be passed into both the logging function and the original branching instruction. The de-instrumentation pass would then need to find this local and attempt to undo the data flow change. Simplify all of this by having the logging function return the condition value so it can interpose between the condition and the branch without any new locals. De-instrumentation can now just replace the call to the log function with its condition parameter.
To allow further simplification, also change the order of parameters to the logging function so the condition value is the first parameter. This ensures that we don't need to introduce a scratch local even when the condition is a
pop, because the pop will remain the leftmost leaf expression in the catch body.