[DONOTCOMMIT] Assert deoptimizer offset [v8/v8 : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1372738a710000
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/152414de710000
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/163fc711710000
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1522a7b2710000
Related details
- Maksim Ivanov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14956392710000
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf-pgo/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/142d9e15710000
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 1 comment![]( "Open in Gerrit")
Looks neutral on benchmarks. PTAL; thanks!
Related details
- Samuel Groß
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 4 comments![]( "Open in Gerrit")
Thanks! Overall this is a nice hardening, but I wonder if this is the best place for it to go. +Leszek as 2nd reviewer and for input on that question.
CHECK_IMPLIES(bytecode_offset != -1,When is the offset -1?
CHECK_IMPLIES(bytecode_offset != -1,I'm not 100% sure that this is the best place for this CHECK to go. Ideally it would be right where we actually jump to the offset. Although I think that would then actually be in a special builtin (InterpreterEnterAtBytecode and friends?). Maybe something for @les...@chromium.org to answer.
CHECK_IMPLIES(bytecode_offset != -1,Maybe add a comment that this is a defense-in-depth check to make sure that we always deopt to a valid offset, and potentially add a link to the bug report as example for what this would mitigate.
Related details
- Leszek Swirski
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
CHECK_IMPLIES(bytecode_offset != -1,Leszek SwirskiI'm not 100% sure that this is the best place for this CHECK to go. Ideally it would be right where we actually jump to the offset. Although I think that would then actually be in a special builtin (InterpreterEnterAtBytecode and friends?). Maybe something for @les...@chromium.org to answer.
I'm not sure it provides any value here at all to be honest. The bugs this CL links are about reading from the TranslatedFrame in other locations so this fix wouldn't have caught those issues. Putting it in `InterpreterEnterAtBytecode` might help, though as you say it's a special builtin and it might be tricky to call into the verification from there since it's doing all the stack setup and putting things into the right registers.
How about instead walking the stack generated by the Deoptimizer once it completes, and verifying the unoptimized frames there?
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 2 comments![]( "Open in Gerrit")
CHECK_IMPLIES(bytecode_offset != -1,Maksim IvanovWhen is the offset -1?
This is reached in some tests, e.g., deopt-pretenure.js:
// Trigger pretenuring decision change at entry, deopting at bytecode offset -1.
https://source.chromium.org/chromium/chromium/src/+/main:v8/test/mjsunit/compiler/deopt-pretenure.js;l=20;drc=90bd0dab38246b701aba31b0e747dd630e46be90
CHECK_IMPLIES(bytecode_offset != -1,Leszek SwirskiI'm not 100% sure that this is the best place for this CHECK to go. Ideally it would be right where we actually jump to the offset. Although I think that would then actually be in a special builtin (InterpreterEnterAtBytecode and friends?). Maybe something for @les...@chromium.org to answer.
I'm not sure it provides any value here at all to be honest. The bugs this CL links are about reading from the TranslatedFrame in other locations so this fix wouldn't have caught those issues. Putting it in `InterpreterEnterAtBytecode` might help, though as you say it's a special builtin and it might be tricky to call into the verification from there since it's doing all the stack setup and putting things into the right registers.
How about instead walking the stack generated by the Deoptimizer once it completes, and verifying the unoptimized frames there?
When testing with the repro from crbug.com/474311222 (and crrev.com/c/7414885 reverted), the CHECK here does catch the issue and prevent the problem reported by ASan.
But, yeah, I'm not really familiar with the code so if there's a better place for such kind of check - let me explore those alternatives you suggested!
Related details
- Leszek Swirski
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 2 comments![]( "Open in Gerrit")
CHECK_IMPLIES(bytecode_offset != -1,I'm not 100% sure that this is the best place for this CHECK to go. Ideally it would be right where we actually jump to the offset. Although I think that would then actually be in a special builtin (InterpreterEnterAtBytecode and friends?). Maybe something for @les...@chromium.org to answer.
Maksim IvanovI'm not sure it provides any value here at all to be honest. The bugs this CL links are about reading from the TranslatedFrame in other locations so this fix wouldn't have caught those issues. Putting it in `InterpreterEnterAtBytecode` might help, though as you say it's a special builtin and it might be tricky to call into the verification from there since it's doing all the stack setup and putting things into the right registers.
How about instead walking the stack generated by the Deoptimizer once it completes, and verifying the unoptimized frames there?
When testing with the repro from crbug.com/474311222 (and crrev.com/c/7414885 reverted), the CHECK here does catch the issue and prevent the problem reported by ASan.
But, yeah, I'm not really familiar with the code so if there's a better place for such kind of check - let me explore those alternatives you suggested!
Ah I think you're right -- I was looking at the `bytecode_array` access here as being a potentially dangerous one, but it's the computation of `catch_handler_pc_offset_` in the `LookupCatchHandler` function that was the original dangerous `bytecode_array` access, and `catch_handler_pc_offset_` is verified here. Nevertheless, it's a bit reactive to check specifically `catch_handler_pc_offset_` here while trusting the `bytecode_array` computed here otherwise, and trusting that both of these will make their way to the translated frame without some unsafe read along the way (indeed, we do actually replace `bytecode_array` a few lines below -- we have a comment explaining why that's ok within our threat space, but it's still dodgy). The closer we get to verifying the actual stack values that the interpreter will read, the better.
CHECK_IMPLIES(bytecode_offset != -1,Maksim IvanovWhen is the offset -1?
This is reached in some tests, e.g., deopt-pretenure.js:
// Trigger pretenuring decision change at entry, deopting at bytecode offset -1.
https://source.chromium.org/chromium/chromium/src/+/main:v8/test/mjsunit/compiler/deopt-pretenure.js;l=20;drc=90bd0dab38246b701aba31b0e747dd630e46be90
yeah -1 is a marker for function entry, you can use `kFunctionEntryBytecodeOffset` to be clearer.
Related details
- Maksim Ivanov
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf-pgo/jetstream failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/102e137f710000
Related details
- Maksim Ivanov
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |