Simplify branch hint instrumentation#8624
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.
4356c25 to
1456ddf
Compare
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. |
|
I just saw another crash even after the fix. |
|
Maybe it needs to be hacked to use text reduction. Or maybe it depends on some very stacky code that any round tripping destroys. You could try using wasm-tools to get an exact text dump and then reducing with AI assistance. Maybe a nop got in between the logging and the branch somehow and caused the fallthrough to be a get of a temp variable, causing the hint deletion to fail? FWIW I ran about 140k iterations of just the branch hint handler and didn't see anything. |
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.