Bug 230823

Summary: Run backwards propagation before we prune the graph after ForceOSRExit nodes in BytecodeParser
Product: WebKit Reporter: Lukas Bernhard <lukas.bernhard>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-watchlist, keith_miller, mark.lam, msaboff, product-security, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
test EWS
ews-feeder: commit-queue-
test EWS
none
test EWS
ews-feeder: commit-queue-
test EWS
none
patch
rmorisset: review+, ews-feeder: commit-queue-
patch for landing
none
patch for landing
none
followup patch
ysuzuki: review+
followup patch for landing none

Lukas Bernhard
Reported 2021-09-27 01:40:12 PDT
Differential testing identifies the following samples to trigger a miscomputation in FTL. Tested on 29c8d02c3b11c096cc67d89e5cfe8c16be42b3b7 (Fri Sep 24 09:39:18 2021 +0000) Release/bin/jsc --validateOptions=true --useConcurrentJIT=false --useConcurrentGC=false --thresholdForJITSoon=10 --thresholdForJITAfterWarmUp=10 --thresholdForOptimizeAfterWarmUp=100 --thresholdForOptimizeAfterLongWarmUp=100 --thresholdForOptimizeSoon=100 --thresholdForFTLOptimizeAfterWarmUp=1000 --thresholdForFTLOptimizeSoon=1000 --validateBCE=true --useFTLJIT=true diff.js function main() { let v38; let v40; async function v24() { const v33 = false; const v34 = -v33; const v37 = typeof search; const v39 = v38 ? v30 : 1; v40 = v34; for(let v41 = 0; v41 != 100000; v41++) { } } [1,1,1].filter(v24); print(Object.is(v40, 0)); // v40 should be -0, but is 0 in FTL (hence prints true in FTL, false without FTL or in v8) } main();
Attachments
test EWS (1.51 KB, patch)
2021-09-29 21:15 PDT, Saam Barati
ews-feeder: commit-queue-
test EWS (6.33 KB, patch)
2021-09-30 17:36 PDT, Saam Barati
no flags
test EWS (6.23 KB, patch)
2021-09-30 17:43 PDT, Saam Barati
ews-feeder: commit-queue-
test EWS (6.85 KB, patch)
2021-09-30 20:27 PDT, Saam Barati
no flags
patch (16.46 KB, patch)
2021-10-01 11:36 PDT, Saam Barati
rmorisset: review+
ews-feeder: commit-queue-
patch for landing (16.37 KB, patch)
2021-10-01 12:04 PDT, Saam Barati
no flags
patch for landing (16.83 KB, patch)
2021-10-05 14:13 PDT, Saam Barati
no flags
followup patch (4.29 KB, patch)
2021-10-07 12:40 PDT, Saam Barati
ysuzuki: review+
followup patch for landing (4.44 KB, patch)
2021-10-08 17:56 PDT, Saam Barati
no flags
Radar WebKit Bug Importer
Comment 1 2021-09-27 01:40:24 PDT
Saam Barati
Comment 2 2021-09-29 21:10:45 PDT
Lukas, your fuzzer is producing some really amazing spec implementation bugs. Please keep the bug reports coming.
Saam Barati
Comment 3 2021-09-29 21:12:50 PDT
The bug here is the code inside the bytecode parser that does liveness preservation for all live values after a ForceOSRExit does not properly keep around the program in such a way that the backwards prop flags are properly maintained. Specifically, we end up with a program like: ``` a: ArithNegate(...) SetLocal(@a, loc6) PhantomLocal(loc6) ``` But, this doesn't tell backwards prop that we care about negative zero. I'm beginning to think that instead of all PhantomLocals, we should just emit Phantom(GetLocal) for all live things that aren't flushed.
Saam Barati
Comment 4 2021-09-29 21:15:05 PDT
Created attachment 439698 [details] test EWS
Saam Barati
Comment 5 2021-09-29 21:17:35 PDT
FWIW, this is both a DFG/FTL bug. This test just happens to require the FTL because it'll OSR exit with a bad constant value because of this bad code inside bytecode parser.
Saam Barati
Comment 6 2021-09-30 17:36:25 PDT
Created attachment 439802 [details] test EWS
Saam Barati
Comment 7 2021-09-30 17:43:26 PDT
Created attachment 439806 [details] test EWS
Saam Barati
Comment 8 2021-09-30 20:27:05 PDT
Created attachment 439815 [details] test EWS
Saam Barati
Comment 9 2021-10-01 11:36:26 PDT
Robin Morisset
Comment 10 2021-10-01 11:53:16 PDT
Comment on attachment 439885 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=439885&action=review r=me. Overall I'm very happy to see that the ugly hack at the end of BytecodeParser which removes some of the code after OSRExits but not all is at least partly gone. > Source/JavaScriptCore/ChangeLog:19 > + That way, the phase sees the graph as if its an IR over the whole bytecode nit: its => it's > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:50 > + for (size_t i = 0; i < m_graph.numBlocks(); ++i) { trivial simplification: you can do ``` for (BasicBlock* block : m_graph.blocksInNaturalOrder()) { m_flagsAtHead[block] = ... ``` blocksInNaturalOrder is exactly this pattern of iterating them in order and skipping any null one. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:9027 > + performBackwardsPropagation(m_graph); Would it be possible/safe to flash a GC safe point before this? While backwards propagation is pretty quick, the bytecode parser is already responsible for many of our worst GC pauses, so I'd really like to avoid making its impact on the GC any worse.
Saam Barati
Comment 11 2021-10-01 11:59:07 PDT
Comment on attachment 439885 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=439885&action=review >> Source/JavaScriptCore/ChangeLog:19 >> + That way, the phase sees the graph as if its an IR over the whole bytecode > > nit: its => it's will fix. >> Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:50 >> + for (size_t i = 0; i < m_graph.numBlocks(); ++i) { > > trivial simplification: you can do > ``` > for (BasicBlock* block : m_graph.blocksInNaturalOrder()) { > m_flagsAtHead[block] = ... > ``` > blocksInNaturalOrder is exactly this pattern of iterating them in order and skipping any null one. will fix. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:9027 >> + performBackwardsPropagation(m_graph); > > Would it be possible/safe to flash a GC safe point before this? While backwards propagation is pretty quick, the bytecode parser is already responsible for many of our worst GC pauses, so I'd really like to avoid making its impact on the GC any worse. I'm not 100% sure if it's safe. My guess would be yes, but it requires some analysis of what happens in bytecode parser after this code runs. If we do it, I think it should be done in its own patch.
Saam Barati
Comment 12 2021-10-01 12:04:47 PDT
Created attachment 439894 [details] patch for landing
Saam Barati
Comment 13 2021-10-05 14:13:32 PDT
Created attachment 440264 [details] patch for landing
EWS
Comment 14 2021-10-06 08:49:50 PDT
Committed r283623 (242575@main): <https://commits.webkit.org/242575@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440264 [details].
Saam Barati
Comment 15 2021-10-06 19:03:15 PDT
Comment on attachment 440264 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=440264&action=review > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:225 > + variableAccessData->mergeFlags(flagsRef & ~NodeBytecodeUsesAsInt); // We don't care about cross-block uses-as-int for this. I think this logic is actually slightly off, since we incorporate the value from m_currentFlags in SetLocal. I was trying to avoid having a separate bit to represent "is alive". But, I should just add that bit.
Saam Barati
Comment 16 2021-10-07 12:40:29 PDT
Created attachment 440527 [details] followup patch
Saam Barati
Comment 17 2021-10-07 12:45:36 PDT
Doing a follow up
Yusuke Suzuki
Comment 18 2021-10-08 13:07:54 PDT
Comment on attachment 440527 [details] followup patch View in context: https://bugs.webkit.org/attachment.cgi?id=440527&action=review r=me > Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:218 > + static constexpr NodeFlags VariableIsUsed = 1 << (1 + WTF::getMSBSetConstexpr(NodeBytecodeBackPropMask)); Can we have a static_assert that `1 << (1 + WTF::getMSBSetConstexpr(NodeBytecodeBackPropMask))` does not exceed NodeFlags size?
Saam Barati
Comment 19 2021-10-08 17:56:07 PDT
Created attachment 440695 [details] followup patch for landing
EWS
Comment 20 2021-10-08 20:30:00 PDT
Committed r283862 (242739@main): <https://commits.webkit.org/242739@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440695 [details].
Note You need to log in before you can comment on or make changes to this bug.