[webkitscmpy] Refactor PR branch management#9
Closed
JonWBedard wants to merge 1 commit into
Closed
Conversation
https://bugs.webkit.org/show_bug.cgi?id=230432 <rdar://problem/83258413> Reviewed by NOBODY (OOPS!). * Scripts/libraries/webkitscmpy/webkitscmpy/program/branch.py: (Branch): Specify that the 'eng' prefix is for pull-requests. (Branch.normalize_branch_name): Normalized branch names should consider other developement prefixes. (Branch.editable): Check if a branch is editable. Only includes development branches at the moment, will include commit-queue in the near future. (Branch.branch_point): Moved from pull_request.py. (Branch.main): (Branch.normalize_issue): Renamed normalize_branch_name. * Scripts/libraries/webkitscmpy/webkitscmpy/program/pull.py: (Pull.main): branch_point is now owned Branch command. * Scripts/libraries/webkitscmpy/webkitscmpy/program/pull_request.py: (PullRequest.main): Match DEV_BRANCHES instead of PREFIX, branch_point is now owned Branch command. (PullRequest.branch_point): Moved to branch.py.
Member
Author
|
Committed a8cc1fc: https://commits.webkit.org/241851@main |
webkit-early-warning-system
pushed a commit
to hyjorc1/WebKit
that referenced
this pull request
Nov 19, 2022
…a rejected promise https://bugs.webkit.org/show_bug.cgi?id=247785 rdar://102325201 Reviewed by Yusuke Suzuki. Rest parameter should be caught in async function. So, running this JavaScript program should print "caught". ``` async function f(...[[]]) { } f().catch(e => print("caught")); ``` V8 (used console.log) ``` $ node input.js caught ``` GraalJS ``` $ js input.js caught ``` https://tc39.es/ecma262/#sec-async-function-definitions ... AsyncFunctionDeclaration[Yield, Await, Default] : async [no LineTerminator here] function BindingIdentifier[?Yield, ?Await] ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } [+Default] async [no LineTerminator here] function ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } AsyncFunctionExpression : async [no LineTerminator here] function BindingIdentifier[~Yield, +Await]opt ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } ... According to the spec, it indicates `FormalParameters` is used for Async Function, where `FormalParameters` can be converted to `FunctionRestParameter`. https://tc39.es/ecma262/#sec-parameter-lists ... FormalParameters[Yield, Await] : [empty] FunctionRestParameter[?Yield, ?Await] FormalParameterList[?Yield, ?Await] FormalParameterList[?Yield, ?Await] , FormalParameterList[?Yield, ?Await] , FunctionRestParameter[?Yield, ?Await] ... And based on RS: EvaluateAsyncFunctionBody, it will invoke the promise.reject callback function with abrupt value ([[value]] of non-normal completion record). https://tc39.es/ecma262/#sec-runtime-semantics-evaluateasyncfunctionbody ... 2. Let declResult be Completion(FunctionDeclarationInstantiation(functionObject, argumentsList)). 3. If declResult is an abrupt completion, then a. Perform ! Call(promiseCapability.[[Reject]], undefined, « declResult.[[Value]] »). ... In that case, any non-normal results of evaluating rest parameters should be caught and passed to the reject callback function. To resolve this problem, we should allow the emitted RestParameterNode be wrapped by the catch handler for promise. However, we should remove `m_restParameter` and emit rest parameter byte code in `initializeDefaultParameterValuesAndSetupFunctionScopeStack` if we can prove that change has no side effect. In that case, we can only use one exception handler. Current fix is to add another exception handler. And move the handler byte codes to the bottom of code block in order to make other byte codes as much compact as possible. Input: ``` async function f(arg0, ...[[]]) { } f(); ``` Dumped Byte Codes: ``` ... bb#2 Predecessors: [ WebKit#1 ] [ 20] mov dst:loc9, src:<JSValue()>(const0) ... bb#3 Predecessors: [ WebKit#2 ] [ 29] get_rest_length dst:loc11, numParametersToSkip:1 ... bb#12 Predecessors: [ WebKit#8 WebKit#9 WebKit#10 ] [ 138] new_func_exp dst:loc10, scope:loc4, functionDecl:0 ... bb#13 Predecessors: [ ] [ 170] catch exception:loc10, thrownValue:loc8 [ 174] jmp targetLabel:8(->182) Successors: [ WebKit#15 ] bb#14 Predecessors: [ WebKit#7 WebKit#11 ] [ 176] catch exception:loc10, thrownValue:loc8 [ 180] jmp targetLabel:2(->182) Successors: [ WebKit#15 ] bb#15 Predecessors: [ WebKit#13 WebKit#14 ] [ 182] mov dst:loc12, src:Undefined(const1) ... Exception Handlers: 1: { start: [ 20] end: [ 29] target: [ 170] } synthesized catch 2: { start: [ 29] end: [ 138] target: [ 176] } synthesized catch ``` * JSTests/stress/catch-rest-parameter.js: Added. (throwError): (shouldThrow): (async f): (throwError.async f): (throwError.async let): (async let): (x.async f): (x): (async shouldThrow): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::BytecodeGenerator): (JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: Canonical link: https://commits.webkit.org/256864@main
cdumez
pushed a commit
to cdumez/WebKit
that referenced
this pull request
Jan 15, 2023
Fix TypeError in parameter destructuring in async function should be a rejected promise
https://bugs.webkit.org/show_bug.cgi?id=247785
rdar://102325201
Reviewed by Yusuke Suzuki.
Rest parameter should be caught in async function. So, running this
JavaScript program should print "caught".
```
async function f(...[[]]) { }
f().catch(e => print("caught"));
```
V8 (used console.log)
```
$ node input.js
caught
```
GraalJS
```
$ js input.js
caught
```
https://tc39.es/ecma262/#sec-async-function-definitions
...
AsyncFunctionDeclaration[Yield, Await, Default] :
async [no LineTerminator here] function BindingIdentifier[?Yield, ?Await] ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody }
[+Default] async [no LineTerminator here] function ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody }
AsyncFunctionExpression :
async [no LineTerminator here] function BindingIdentifier[~Yield, +Await]opt ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody }
...
According to the spec, it indicates `FormalParameters` is used for Async
Function, where `FormalParameters` can be converted to `FunctionRestParameter`.
https://tc39.es/ecma262/#sec-parameter-lists
...
FormalParameters[Yield, Await] :
[empty]
FunctionRestParameter[?Yield, ?Await]
FormalParameterList[?Yield, ?Await]
FormalParameterList[?Yield, ?Await] ,
FormalParameterList[?Yield, ?Await] , FunctionRestParameter[?Yield, ?Await]
...
And based on RS: EvaluateAsyncFunctionBody, it will invoke the promise.reject
callback function with abrupt value ([[value]] of non-normal completion record).
https://tc39.es/ecma262/#sec-runtime-semantics-evaluateasyncfunctionbody
...
2. Let declResult be Completion(FunctionDeclarationInstantiation(functionObject, argumentsList)).
3. If declResult is an abrupt completion, then
a. Perform ! Call(promiseCapability.[[Reject]], undefined, « declResult.[[Value]] »).
...
In that case, any non-normal results of evaluating rest parameters should be
caught and passed to the reject callback function.
To resolve this problem, we should allow the emitted RestParameterNode be wrapped
by the catch handler for promise. However, we should remove `m_restParameter` and
emit rest parameter byte code in `initializeDefaultParameterValuesAndSetupFunctionScopeStack`
if we can prove that change has no side effect. In that case, we can only use one
exception handler.
Current fix is to add another exception handler. And move the handler byte codes to
the bottom of code block in order to make other byte codes as much compact as possible.
Input:
```
async function f(arg0, ...[[]]) { }
f();
```
Dumped Byte Codes:
```
...
bb#2
Predecessors: [ WebKit#1 ]
[ 20] mov dst:loc9, src:<JSValue()>(const0)
...
bb#3
Predecessors: [ WebKit#2 ]
[ 29] get_rest_length dst:loc11, numParametersToSkip:1
...
bb#12
Predecessors: [ WebKit#8 WebKit#9 WebKit#10 ]
[ 138] new_func_exp dst:loc10, scope:loc4, functionDecl:0
...
bb#13
Predecessors: [ ]
[ 170] catch exception:loc10, thrownValue:loc8
[ 174] jmp targetLabel:8(->182)
Successors: [ WebKit#15 ]
bb#14
Predecessors: [ WebKit#7 WebKit#11 ]
[ 176] catch exception:loc10, thrownValue:loc8
[ 180] jmp targetLabel:2(->182)
Successors: [ WebKit#15 ]
bb#15
Predecessors: [ WebKit#13 WebKit#14 ]
[ 182] mov dst:loc12, src:Undefined(const1)
...
Exception Handlers:
1: { start: [ 20] end: [ 29] target: [ 170] } synthesized catch
2: { start: [ 29] end: [ 138] target: [ 176] } synthesized catch
```
* JSTests/stress/catch-rest-parameter.js: Added.
(throwError):
(shouldThrow):
(async f):
(throwError.async f):
(throwError.async let):
(async let):
(x.async f):
(x):
(async shouldThrow):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:
Canonical link: https://commits.webkit.org/256864@main
Canonical link: https://commits.webkit.org/252432.994@safari-7614-branch
webkit-commit-queue
pushed a commit
to Constellation/WebKit
that referenced
this pull request
Jan 24, 2023
https://bugs.webkit.org/show_bug.cgi?id=251063 rdar://104585575 Reviewed by Mark Lam and Justin Michaud. This patch enhances CallFrame::dump to support wasm frames in btjs stacktrace. The example is as follows. frame #0: 0x00000001035fca78 JavaScriptCore`JSC::functionBreakpoint(globalObject=0x000000012f410068, callFrame=0x000000016fdfa9d0) at JSDollarVM.cpp:2273:9 [opt] frame WebKit#1: 0x000000010ec44204 0x10eccc5dc frame WebKit#2: 0x000000010eccc5dc callback#Dwaxn6 [Baseline bc#50](Undefined) frame WebKit#3: 0x000000010ec4ca84 wasm-stub [WasmToJS](Wasm::Instance: 0x10d29da40) frame WebKit#4: 0x000000010ed0c060 <?>.wasm-function[1] [OMG](Wasm::Instance: 0x10d29da40) frame WebKit#5: 0x000000010ed100d0 jsToWasm#CWTx6k [FTL bc#22](Cell[JSModuleEnvironment]: 0x12f524540, Cell[WebAssemblyFunction]: 0x10d06a3a8, 1, 2, 3) frame WebKit#6: 0x000000010ec881b0 #D5ymZE [Baseline bc#733](Undefined, Cell[Generator]: 0x12f55c180, 1, Cell[Object]: 0x12f69dfc0, 0, Cell[JSLexicalEnvironment]: 0x12f52cee0) frame WebKit#7: 0x000000010ec3c008 asyncFunctionResume#A4ayYg [LLInt bc#49](Undefined, Cell[Generator]: 0x12f55c180, Cell[Object]: 0x12f69dfc0, 0) frame WebKit#8: 0x000000010ec3c008 promiseReactionJobWithoutPromise#D0yDF1 [LLInt bc#25](Undefined, Cell[Function]: 0x12f44f3c0, Cell[Object]: 0x12f69dfc0, Cell[Generator]: 0x12f55c180) frame WebKit#9: 0x000000010ec80ec0 promiseReactionJob#EdShZz [Baseline bc#74](Undefined, Undefined, Cell[Function]: 0x12f44f3c0, Cell[Object]: 0x12f69dfc0, Cell[Generator]: 0x12f55c180) frame WebKit#10: 0x000000010ec3c728 frame WebKit#11: 0x0000000103137560 JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) [inlined] JSC::JITCode::execute(this=<unavailable>, vm=<unavailable>, protoCallFrame=<unavailable>) at JITCodeInlines.h:42:38 [opt] frame WebKit#12: 0x0000000103137524 JavaScriptCore`JSC::Interpreter::executeCall(this=<unavailable>, lexicalGlobalObject=<unavailable>, function=<unavailable>, callData=<unavailable>, thisValue=<unavailable>, args=<unavailable>) at Interpreter.cpp:1093:27 [opt] frame WebKit#13: 0x000000010349d6d0 JavaScriptCore`JSC::runJSMicrotask(globalObject=0x000000012f410068, identifier=(m_identifier = 81), job=JSValue @ x22, argument0=JSValue @ x26, argument1=JSValue @ x25, argument2=<unavailable>, argument3=<unavailable>) at JSMicrotask.cpp:98:9 [opt] frame WebKit#14: 0x00000001039dfc54 JavaScriptCore`JSC::VM::drainMicrotasks() (.cold.1) at VM.cpp:0:9 [opt] frame WebKit#15: 0x00000001035e58a4 JavaScriptCore`JSC::VM::drainMicrotasks() [inlined] JSC::MicrotaskQueue::dequeue(this=<unavailable>) at VM.cpp:0:9 [opt] frame WebKit#16: 0x00000001035e5894 JavaScriptCore`JSC::VM::drainMicrotasks(this=0x000000012f000000) at VM.cpp:1255:46 [opt] ... * Source/JavaScriptCore/interpreter/CallFrame.cpp: (JSC::CallFrame::dump const): Canonical link: https://commits.webkit.org/259262@main
CGQAQ
pushed a commit
to CGQAQ/WebKit
that referenced
this pull request
Aug 25, 2023
Add debug build of WebKit
WebKit-Jenner
pushed a commit
that referenced
this pull request
May 14, 2024
…g LICM to miscompile https://bugs.webkit.org/show_bug.cgi?id=271435 rdar://124506508 Reviewed by Yusuke Suzuki. Consider the following example: ============================================================================================================ FIRST SLEEP (before performCFA) D@80:< 10:-> JSConstant(JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Final, Weak:Object: 0x13a0e8140 with butterfly 0x0(base=0xfffffffffffffff8) (Structure %AJ:Object), StructureID: 40640, bc#0, ExitValid) D@126:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(x)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], [], offset = 0>, seenInJIT = true), W:SideState, bc#112, ExitValid) D@128:<!0:-> CheckStructure(Cell:D@80, MustGen, [%AJ:Object], R:JSCell_structureID, Exits, bc#112, ExitValid) D@133:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(toJSON),cell:(String (atomic),8Bit:(1),length:(6): toJSON, StructureID: 16976)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], [], offset = 1>, seenInJIT = true), W:SideState, bc#118, ExitValid) D@136:< 4:-> GetByOffset(KnownCell:D@80, KnownCell:D@80, JS|PureNum|NeedsNaNOrInfinity|UseAsOther|ReallyWantsInt, BoolInt32, id6{toJSON}, 1, R:NamedProperties(6), bc#118, ExitValid) predicting BoolInt32 D@138:<!0:-> Check(Check:Int32:D@136, MustGen, Exits, bc#118, exit: bc#124, ExitValid) D@140:<!0:-> Branch(Boolean:D@35, MustGen, T:#9/w:10.000000, F:#12/w:10.000000, W:SideState, bc#124, ExitValid) D@4:< 1:-> GetButterfly(Cell:D@104, Storage|PureInt, R:JSObject_butterfly, bc#127, ExitValid) D@1:<!1:-> CheckInBounds(Int32:D@136, KnownInt32:D@151, JS|MustGen|PureInt, Int32, Exits, bc#127, ExitValid) D@143:< 3:-> GetByVal(KnownCell:D@104, Int32:Kill:D@136, Check:Untyped:Kill:D@4, Check:Untyped:Kill:D@1, JS|VarArgs|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Contiguous+OriginalCopyOnWriteArray+InBoundsSaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedContiguousProperties, Exits, bc#127, ExitValid) predicting StringIdent %AJ:Object = 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8] Execution: AI GetByOffset D@136 AI says (BoolInt32, Int32: 0, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x0(base=0xfffffffffffffff8) (Structure 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]), StructureID: 40640, 1:StructuresAreWatched) state StructuresAreWatched AI CheckInBounds D@1 AI says left Int32:D@136 is Int32: 0 SECOND SLEEP (after performCFA, before performConstantFolding) Note that the jsconstant has a structure transition at this point. D@80:< 10:-> JSConstant(JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Final, Weak:Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002380) (Structure %AR:Object), StructureID: 40976, bc#0, ExitValid) D@126:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(x)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], [], offset = 0>, seenInJIT = true), W:SideState, bc#112, ExitValid) D@128:<!0:-> CheckStructure(Cell:D@80, MustGen, [%AR:Object], R:JSCell_structureID, Exits, bc#112, ExitValid) D@133:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(toJSON),cell:(String (atomic),8Bit:(1),length:(6): toJSON, StructureID: 16976)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], [], offset = 1>, seenInJIT = true), W:SideState, bc#118, ExitValid) D@136:< 4:-> GetByOffset(KnownCell:D@80, KnownCell:D@80, JS|PureNum|NeedsNaNOrInfinity|UseAsOther|ReallyWantsInt, BoolInt32, id6{toJSON}, 1, R:NamedProperties(6), bc#118, ExitValid) predicting BoolInt32 D@4:< 1:-> GetButterfly(Cell:D@104, Storage|PureInt, R:JSObject_butterfly, bc#127, ExitValid) D@1:<!1:-> CheckInBounds(Int32:D@136, KnownInt32:D@151, JS|MustGen|PureInt, Int32, Exits, bc#127, ExitValid) D@143:< 3:-> GetByVal(KnownCell:D@104, Int32:Kill:D@136, Check:Untyped:Kill:D@4, Check:Untyped:Kill:D@1, JS|VarArgs|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Contiguous+OriginalCopyOnWriteArray+InBoundsSaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedContiguousProperties, Exits, bc#127, ExitValid) predicting StringIdent %AR:Object = 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8] %B6:Object = 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)] Execution: AI GetByOffset D@136 AI says (HeapTop, TOP, TOP, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002360) (Structure 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)]), StructureID: 40976, 1:StructuresAreWatched) state StructuresAreWatched GetByOffset D@136 AI says (HeapTop, TOP, TOP, 1:StructuresAreWatched) CheckInBounds D@1 AI says left Int32:D@136 is Int32: 0 AI GetByOffset D@136 AI says (HeapTop, TOP, TOP, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002360) (Structure 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)]), StructureID: 40976, 1:StructuresAreWatched) state StructuresAreWatched SLEEP DONE ============================================================================================================ The constant folding phase chooses to fold the CheckInBounds, but not the GetByOffset. At this point, this is still correct (although sub-optimal). 1) Why does AI disagree in these two places? The constant folding phase doesn't re-run AI. It runs it from top to bottom on certain blocks only. In this example, The CheckInBounds AI proof is read directly from the block, but the GetByOffset has its value computed. 1) Why can the JSConstant's structure change without triggering a watchpoint? The constant remains constant. We never used the fact that that it had a certain structure anywhere, our proofs stem from the fact that we have a CheckStructure. 1) Why does the re-run AI pass in performConstantFolding not predict the GetByOffset to be constant? The structure change causes GetPropertyConcurrently to fail to get the value concurrently. We must assume that it is always safe to produce a more conservative result in this phase. Note though that if the phase returned the same value as the first time around, that would still have been correct! The answer to this question didn't change, we just lost the ability to compute it. ============================================================================================================ Why this is a problem This is a classic example of a broad class of bugs affecting the JIT. Different passes can see different values as the mutator changes the object graph, even for the same pass. Normally this is fine, because the compiler is always narrowing its assumptions. Specifically, with each pass we assume more and more detailed things about the code, and guard against these assumptions being wrong either with watchpoints or runtime checks. In this example, we see that we CheckStructure. Then, as a result, we can elide nodes that are dominated by that check (like the GetByOffset or the CheckInBounds). As long as we never loosen that assumption again, we are fine. In this example, our CFA pass assumes that the GetByOffset is constant. The Constant Folding phase then assumes sometimes that it is constant, and sometimes that it is not. This puts us in opposition to another principle, that is the idea that we should always be able to answer any question asked of us conservatively and be safe. Up until this point, both of these ideas are holding true. Unfortunately, we also need LICM. LICM needs to run after many assumptions have already been made, and it dramatically loosens assumptions. In this example, LICM comes along and hoists the GetByVal(GetByOffset()) above the CheckStructure. If we had indeed constant folded the GetByOffset too, we would be fine to do. We should always be able to avoid constant folding safely. LICM should be able to hoist constant values safely. ============================================================================================================ How to fix this generally 1) If AI says something is constant, just make it constant then. This is the simplest solution, and should just work. This makes sure that what AI says is true, even if LICM moves stuff around. This would require some re-work of the AI phase though. 1) LCIM should see that this isn't safe to move The effects here are super specific. If LICM asked the question "If I move this, is this still safe to execute?" it would have answered "no" in this case (without the structure check). Of course, if we hadn't removed the CheckInBounds, the answer would be "yes," which is also fine. One could imagine that this analysis would be pretty difficult. 1) Always run the constant folder on each block. ``` // This method is evil - it causes a huge maintenance headache and there is a gross amount of // code devoted to it. It would be much nicer to just always run the constant folder on each // block. But, the last time we did it, it was a 1% SunSpider regression: // https://bugs.webkit.org/show_bug.cgi?id=133947 // So, we should probably keep this method. void setShouldTryConstantFolding(bool tryConstantFolding) { m_shouldTryConstantFolding = tryConstantFolding; } ``` This would fix the issue though, as a failure to prove something at any point in time would not permit that proof to be used later on. This patch chooses the third option. This appears to be perf-neutral on modern hardware on JS2/3 and SP2/3. * JSTests/stress/get-by-val-hoist-above-structure.js: Added. (opt): (createObjectOfS1): (createObjectOfS2): (main): * Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp: (JSC::DFG::ConstantFoldingPhase::foldConstants): Canonical link: https://commits.webkit.org/272448.796@safari-7618-branch
aperezdc
pushed a commit
that referenced
this pull request
May 15, 2024
…kit.org/show_bug.cgi?id=271435 DFG Constant Folding phase can see inconsistent view of world, causing LICM to miscompile https://bugs.webkit.org/show_bug.cgi?id=271435 rdar://124506508 Reviewed by Yusuke Suzuki. Consider the following example: ============================================================================================================ FIRST SLEEP (before performCFA) D@80:< 10:-> JSConstant(JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Final, Weak:Object: 0x13a0e8140 with butterfly 0x0(base=0xfffffffffffffff8) (Structure %AJ:Object), StructureID: 40640, bc#0, ExitValid) D@126:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(x)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], [], offset = 0>, seenInJIT = true), W:SideState, bc#112, ExitValid) D@128:<!0:-> CheckStructure(Cell:D@80, MustGen, [%AJ:Object], R:JSCell_structureID, Exits, bc#112, ExitValid) D@133:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(toJSON),cell:(String (atomic),8Bit:(1),length:(6): toJSON, StructureID: 16976)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], [], offset = 1>, seenInJIT = true), W:SideState, bc#118, ExitValid) D@136:< 4:-> GetByOffset(KnownCell:D@80, KnownCell:D@80, JS|PureNum|NeedsNaNOrInfinity|UseAsOther|ReallyWantsInt, BoolInt32, id6{toJSON}, 1, R:NamedProperties(6), bc#118, ExitValid) predicting BoolInt32 D@138:<!0:-> Check(Check:Int32:D@136, MustGen, Exits, bc#118, exit: bc#124, ExitValid) D@140:<!0:-> Branch(Boolean:D@35, MustGen, T:#9/w:10.000000, F:#12/w:10.000000, W:SideState, bc#124, ExitValid) D@4:< 1:-> GetButterfly(Cell:D@104, Storage|PureInt, R:JSObject_butterfly, bc#127, ExitValid) D@1:<!1:-> CheckInBounds(Int32:D@136, KnownInt32:D@151, JS|MustGen|PureInt, Int32, Exits, bc#127, ExitValid) D@143:< 3:-> GetByVal(KnownCell:D@104, Int32:Kill:D@136, Check:Untyped:Kill:D@4, Check:Untyped:Kill:D@1, JS|VarArgs|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Contiguous+OriginalCopyOnWriteArray+InBoundsSaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedContiguousProperties, Exits, bc#127, ExitValid) predicting StringIdent %AJ:Object = 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8] Execution: AI GetByOffset D@136 AI says (BoolInt32, Int32: 0, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x0(base=0xfffffffffffffff8) (Structure 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]), StructureID: 40640, 1:StructuresAreWatched) state StructuresAreWatched AI CheckInBounds D@1 AI says left Int32:D@136 is Int32: 0 SECOND SLEEP (after performCFA, before performConstantFolding) Note that the jsconstant has a structure transition at this point. D@80:< 10:-> JSConstant(JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Final, Weak:Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002380) (Structure %AR:Object), StructureID: 40976, bc#0, ExitValid) D@126:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(x)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], [], offset = 0>, seenInJIT = true), W:SideState, bc#112, ExitValid) D@128:<!0:-> CheckStructure(Cell:D@80, MustGen, [%AR:Object], R:JSCell_structureID, Exits, bc#112, ExitValid) D@133:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(toJSON),cell:(String (atomic),8Bit:(1),length:(6): toJSON, StructureID: 16976)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], [], offset = 1>, seenInJIT = true), W:SideState, bc#118, ExitValid) D@136:< 4:-> GetByOffset(KnownCell:D@80, KnownCell:D@80, JS|PureNum|NeedsNaNOrInfinity|UseAsOther|ReallyWantsInt, BoolInt32, id6{toJSON}, 1, R:NamedProperties(6), bc#118, ExitValid) predicting BoolInt32 D@4:< 1:-> GetButterfly(Cell:D@104, Storage|PureInt, R:JSObject_butterfly, bc#127, ExitValid) D@1:<!1:-> CheckInBounds(Int32:D@136, KnownInt32:D@151, JS|MustGen|PureInt, Int32, Exits, bc#127, ExitValid) D@143:< 3:-> GetByVal(KnownCell:D@104, Int32:Kill:D@136, Check:Untyped:Kill:D@4, Check:Untyped:Kill:D@1, JS|VarArgs|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Contiguous+OriginalCopyOnWriteArray+InBoundsSaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedContiguousProperties, Exits, bc#127, ExitValid) predicting StringIdent %AR:Object = 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8] %B6:Object = 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)] Execution: AI GetByOffset D@136 AI says (HeapTop, TOP, TOP, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002360) (Structure 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)]), StructureID: 40976, 1:StructuresAreWatched) state StructuresAreWatched GetByOffset D@136 AI says (HeapTop, TOP, TOP, 1:StructuresAreWatched) CheckInBounds D@1 AI says left Int32:D@136 is Int32: 0 AI GetByOffset D@136 AI says (HeapTop, TOP, TOP, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002360) (Structure 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)]), StructureID: 40976, 1:StructuresAreWatched) state StructuresAreWatched SLEEP DONE ============================================================================================================ The constant folding phase chooses to fold the CheckInBounds, but not the GetByOffset. At this point, this is still correct (although sub-optimal). 1) Why does AI disagree in these two places? The constant folding phase doesn't re-run AI. It runs it from top to bottom on certain blocks only. In this example, The CheckInBounds AI proof is read directly from the block, but the GetByOffset has its value computed. 1) Why can the JSConstant's structure change without triggering a watchpoint? The constant remains constant. We never used the fact that that it had a certain structure anywhere, our proofs stem from the fact that we have a CheckStructure. 1) Why does the re-run AI pass in performConstantFolding not predict the GetByOffset to be constant? The structure change causes GetPropertyConcurrently to fail to get the value concurrently. We must assume that it is always safe to produce a more conservative result in this phase. Note though that if the phase returned the same value as the first time around, that would still have been correct! The answer to this question didn't change, we just lost the ability to compute it. ============================================================================================================ Why this is a problem This is a classic example of a broad class of bugs affecting the JIT. Different passes can see different values as the mutator changes the object graph, even for the same pass. Normally this is fine, because the compiler is always narrowing its assumptions. Specifically, with each pass we assume more and more detailed things about the code, and guard against these assumptions being wrong either with watchpoints or runtime checks. In this example, we see that we CheckStructure. Then, as a result, we can elide nodes that are dominated by that check (like the GetByOffset or the CheckInBounds). As long as we never loosen that assumption again, we are fine. In this example, our CFA pass assumes that the GetByOffset is constant. The Constant Folding phase then assumes sometimes that it is constant, and sometimes that it is not. This puts us in opposition to another principle, that is the idea that we should always be able to answer any question asked of us conservatively and be safe. Up until this point, both of these ideas are holding true. Unfortunately, we also need LICM. LICM needs to run after many assumptions have already been made, and it dramatically loosens assumptions. In this example, LICM comes along and hoists the GetByVal(GetByOffset()) above the CheckStructure. If we had indeed constant folded the GetByOffset too, we would be fine to do. We should always be able to avoid constant folding safely. LICM should be able to hoist constant values safely. ============================================================================================================ How to fix this generally 1) If AI says something is constant, just make it constant then. This is the simplest solution, and should just work. This makes sure that what AI says is true, even if LICM moves stuff around. This would require some re-work of the AI phase though. 1) LCIM should see that this isn't safe to move The effects here are super specific. If LICM asked the question "If I move this, is this still safe to execute?" it would have answered "no" in this case (without the structure check). Of course, if we hadn't removed the CheckInBounds, the answer would be "yes," which is also fine. One could imagine that this analysis would be pretty difficult. 1) Always run the constant folder on each block. ``` // This method is evil - it causes a huge maintenance headache and there is a gross amount of // code devoted to it. It would be much nicer to just always run the constant folder on each // block. But, the last time we did it, it was a 1% SunSpider regression: // https://bugs.webkit.org/show_bug.cgi?id=133947 // So, we should probably keep this method. void setShouldTryConstantFolding(bool tryConstantFolding) { m_shouldTryConstantFolding = tryConstantFolding; } ``` This would fix the issue though, as a failure to prove something at any point in time would not permit that proof to be used later on. This patch chooses the third option. This appears to be perf-neutral on modern hardware on JS2/3 and SP2/3. * JSTests/stress/get-by-val-hoist-above-structure.js: Added. (opt): (createObjectOfS1): (createObjectOfS2): (main): * Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp: (JSC::DFG::ConstantFoldingPhase::foldConstants): Canonical link: https://commits.webkit.org/272448.796@safari-7618-branch Canonical link: https://commits.webkit.org/274313.239@webkitglib/2.44
webkit-commit-queue
pushed a commit
to robert-jenner/WebKit
that referenced
this pull request
May 16, 2024
…g LICM to miscompile https://bugs.webkit.org/show_bug.cgi?id=271435 rdar://124506508 Reviewed by Yusuke Suzuki. Consider the following example: ============================================================================================================ FIRST SLEEP (before performCFA) D@80:< 10:-> JSConstant(JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Final, Weak:Object: 0x13a0e8140 with butterfly 0x0(base=0xfffffffffffffff8) (Structure %AJ:Object), StructureID: 40640, bc#0, ExitValid) D@126:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(x)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], [], offset = 0>, seenInJIT = true), W:SideState, bc#112, ExitValid) D@128:<!0:-> CheckStructure(Cell:D@80, MustGen, [%AJ:Object], R:JSCell_structureID, Exits, bc#112, ExitValid) D@133:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(toJSON),cell:(String (atomic),8Bit:(1),length:(6): toJSON, StructureID: 16976)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], [], offset = 1>, seenInJIT = true), W:SideState, bc#118, ExitValid) D@136:< 4:-> GetByOffset(KnownCell:D@80, KnownCell:D@80, JS|PureNum|NeedsNaNOrInfinity|UseAsOther|ReallyWantsInt, BoolInt32, id6{toJSON}, 1, R:NamedProperties(6), bc#118, ExitValid) predicting BoolInt32 D@138:<!0:-> Check(Check:Int32:D@136, MustGen, Exits, bc#118, exit: bc#124, ExitValid) D@140:<!0:-> Branch(Boolean:D@35, MustGen, T:WebKit#9/w:10.000000, F:WebKit#12/w:10.000000, W:SideState, bc#124, ExitValid) D@4:< 1:-> GetButterfly(Cell:D@104, Storage|PureInt, R:JSObject_butterfly, bc#127, ExitValid) D@1:<!1:-> CheckInBounds(Int32:D@136, KnownInt32:D@151, JS|MustGen|PureInt, Int32, Exits, bc#127, ExitValid) D@143:< 3:-> GetByVal(KnownCell:D@104, Int32:Kill:D@136, Check:Untyped:Kill:D@4, Check:Untyped:Kill:D@1, JS|VarArgs|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Contiguous+OriginalCopyOnWriteArray+InBoundsSaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedContiguousProperties, Exits, bc#127, ExitValid) predicting StringIdent %AJ:Object = 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8] Execution: AI GetByOffset D@136 AI says (BoolInt32, Int32: 0, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x0(base=0xfffffffffffffff8) (Structure 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){x:0, toJSON:1}, NonArray, Proto:0x1180348d8]), StructureID: 40640, 1:StructuresAreWatched) state StructuresAreWatched AI CheckInBounds D@1 AI says left Int32:D@136 is Int32: 0 SECOND SLEEP (after performCFA, before performConstantFolding) Note that the jsconstant has a structure transition at this point. D@80:< 10:-> JSConstant(JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Final, Weak:Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002380) (Structure %AR:Object), StructureID: 40976, bc#0, ExitValid) D@126:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(x)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], [], offset = 0>, seenInJIT = true), W:SideState, bc#112, ExitValid) D@128:<!0:-> CheckStructure(Cell:D@80, MustGen, [%AR:Object], R:JSCell_structureID, Exits, bc#112, ExitValid) D@133:<!0:-> FilterGetByStatus(Check:Untyped:D@80, MustGen, (Simple, <id='uid:(toJSON),cell:(String (atomic),8Bit:(1),length:(6): toJSON, StructureID: 16976)', [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], [], offset = 1>, seenInJIT = true), W:SideState, bc#118, ExitValid) D@136:< 4:-> GetByOffset(KnownCell:D@80, KnownCell:D@80, JS|PureNum|NeedsNaNOrInfinity|UseAsOther|ReallyWantsInt, BoolInt32, id6{toJSON}, 1, R:NamedProperties(6), bc#118, ExitValid) predicting BoolInt32 D@4:< 1:-> GetButterfly(Cell:D@104, Storage|PureInt, R:JSObject_butterfly, bc#127, ExitValid) D@1:<!1:-> CheckInBounds(Int32:D@136, KnownInt32:D@151, JS|MustGen|PureInt, Int32, Exits, bc#127, ExitValid) D@143:< 3:-> GetByVal(KnownCell:D@104, Int32:Kill:D@136, Check:Untyped:Kill:D@4, Check:Untyped:Kill:D@1, JS|VarArgs|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Contiguous+OriginalCopyOnWriteArray+InBoundsSaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedContiguousProperties, Exits, bc#127, ExitValid) predicting StringIdent %AR:Object = 0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8] %B6:Object = 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)] Execution: AI GetByOffset D@136 AI says (HeapTop, TOP, TOP, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002360) (Structure 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)]), StructureID: 40976, 1:StructuresAreWatched) state StructuresAreWatched GetByOffset D@136 AI says (HeapTop, TOP, TOP, 1:StructuresAreWatched) CheckInBounds D@1 AI says left Int32:D@136 is Int32: 0 AI GetByOffset D@136 AI says (HeapTop, TOP, TOP, none:StructuresAreClobbered) base: (Final, NonArray, [0x300009ec0:[0x9ec0/40640, Object, (2/2, 0/0){toJSON:1, x:0}, NonArray, Proto:0x1180348d8]], Object: 0x13a0e8140 with butterfly 0x8014002388(base=0x8014002360) (Structure 0x30000a010:[0xa010/40976, Object, (2/2, 1/4){y:64, toJSON:1, x:0}, NonArray, Proto:0x1180348d8, Leaf (Watched)]), StructureID: 40976, 1:StructuresAreWatched) state StructuresAreWatched SLEEP DONE ============================================================================================================ The constant folding phase chooses to fold the CheckInBounds, but not the GetByOffset. At this point, this is still correct (although sub-optimal). 1) Why does AI disagree in these two places? The constant folding phase doesn't re-run AI. It runs it from top to bottom on certain blocks only. In this example, The CheckInBounds AI proof is read directly from the block, but the GetByOffset has its value computed. 1) Why can the JSConstant's structure change without triggering a watchpoint? The constant remains constant. We never used the fact that that it had a certain structure anywhere, our proofs stem from the fact that we have a CheckStructure. 1) Why does the re-run AI pass in performConstantFolding not predict the GetByOffset to be constant? The structure change causes GetPropertyConcurrently to fail to get the value concurrently. We must assume that it is always safe to produce a more conservative result in this phase. Note though that if the phase returned the same value as the first time around, that would still have been correct! The answer to this question didn't change, we just lost the ability to compute it. ============================================================================================================ Why this is a problem This is a classic example of a broad class of bugs affecting the JIT. Different passes can see different values as the mutator changes the object graph, even for the same pass. Normally this is fine, because the compiler is always narrowing its assumptions. Specifically, with each pass we assume more and more detailed things about the code, and guard against these assumptions being wrong either with watchpoints or runtime checks. In this example, we see that we CheckStructure. Then, as a result, we can elide nodes that are dominated by that check (like the GetByOffset or the CheckInBounds). As long as we never loosen that assumption again, we are fine. In this example, our CFA pass assumes that the GetByOffset is constant. The Constant Folding phase then assumes sometimes that it is constant, and sometimes that it is not. This puts us in opposition to another principle, that is the idea that we should always be able to answer any question asked of us conservatively and be safe. Up until this point, both of these ideas are holding true. Unfortunately, we also need LICM. LICM needs to run after many assumptions have already been made, and it dramatically loosens assumptions. In this example, LICM comes along and hoists the GetByVal(GetByOffset()) above the CheckStructure. If we had indeed constant folded the GetByOffset too, we would be fine to do. We should always be able to avoid constant folding safely. LICM should be able to hoist constant values safely. ============================================================================================================ How to fix this generally 1) If AI says something is constant, just make it constant then. This is the simplest solution, and should just work. This makes sure that what AI says is true, even if LICM moves stuff around. This would require some re-work of the AI phase though. 1) LCIM should see that this isn't safe to move The effects here are super specific. If LICM asked the question "If I move this, is this still safe to execute?" it would have answered "no" in this case (without the structure check). Of course, if we hadn't removed the CheckInBounds, the answer would be "yes," which is also fine. One could imagine that this analysis would be pretty difficult. 1) Always run the constant folder on each block. ``` // This method is evil - it causes a huge maintenance headache and there is a gross amount of // code devoted to it. It would be much nicer to just always run the constant folder on each // block. But, the last time we did it, it was a 1% SunSpider regression: // https://bugs.webkit.org/show_bug.cgi?id=133947 // So, we should probably keep this method. void setShouldTryConstantFolding(bool tryConstantFolding) { m_shouldTryConstantFolding = tryConstantFolding; } ``` This would fix the issue though, as a failure to prove something at any point in time would not permit that proof to be used later on. This patch chooses the third option. This appears to be perf-neutral on modern hardware on JS2/3 and SP2/3. * JSTests/stress/get-by-val-hoist-above-structure.js: Added. (opt): (createObjectOfS1): (createObjectOfS2): (main): * Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp: (JSC::DFG::ConstantFoldingPhase::foldConstants): Originally-landed-as: 272448.796@safari-7618-branch (8d5ba1e). rdar://128088091 Canonical link: https://commits.webkit.org/278842@main
philn
added a commit
to philn/WebKit
that referenced
this pull request
Feb 6, 2025
Reviewed by NOBODY (OOPS!).
This was spotted by ASan, the real and imaginary AudioFloatArrays end-up using aligned_alloc() for
their storage, which expects a power-of-two size. Using fftSize / 2 + 1 makes the size
non-power-of-two, the full fftSize being power-of-two (we now have an ASSERT for this).
==1733723==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc75c438bca bp 0x7ffed612c860 sp 0x7ffed612c028 T0)
==1733723==The signal is caused by a WRITE memory access.
==1733723==Hint: address points to the zero page.
#0 0x7fc75c438bca in __memset_avx2_unaligned_erms (/lib64/libc.so.6+0x16dbca) (BuildId: 77c77fee058b19c6f001cf2cb0371ce3b8341211)
WebKit#1 0x2fc4ce in __asan_memset (/var/home/phil/WebKit/local-build-gtk/WebKitBuild/GTK/Release/bin/WebKitWebProcess+0x2fc4ce) (BuildId: e2b0aff0c8fcab48026d63cf711cd70d0aeceb79)
WebKit#2 0x7fc77047599e in WebCore::FFTFrame::FFTFrame(unsigned int) UnifiedSource-3c72abbe-20.cpp
WebKit#3 0x7fc76d41c08b in WebCore::PeriodicWave::createBandLimitedTables(float const*, float const*, unsigned int, WebCore::ShouldDisableNormalization) UnifiedSource-f8afad56-52.cpp
WebKit#4 0x7fc76d41f371 in WebCore::PeriodicWave::generateBasicWaveform(WebCore::PeriodicWave::Type) UnifiedSource-f8afad56-52.cpp
WebKit#5 0x7fc76d41e633 in WebCore::PeriodicWave::createSine(float) UnifiedSource-f8afad56-52.cpp
WebKit#6 0x7fc76d3c2352 in WebCore::BaseAudioContext::periodicWave(WebCore::OscillatorType) UnifiedSource-f8afad56-49.cpp
WebKit#7 0x7fc76d410dbf in WebCore::OscillatorNode::setTypeForBindings(WebCore::OscillatorType) UnifiedSource-f8afad56-52.cpp
WebKit#8 0x7fc76d4102f4 in WebCore::OscillatorNode::create(WebCore::BaseAudioContext&, WebCore::OscillatorOptions const&) UnifiedSource-f8afad56-52.cpp
WebKit#9 0x7fc76d3bc210 in WebCore::BaseAudioContext::createOscillator() UnifiedSource-f8afad56-49.cpp
WebKit#10 0x7fc76acbc743 in WebCore::jsBaseAudioContextPrototypeFunction_createOscillator(JSC::JSGlobalObject*, JSC::CallFrame*) UnifiedSource-3a52ce78-11.cpp
WebKit#11 0x7fc705c10037 (<unknown module>)
* Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:
(WebCore::FFTFrame::FFTFrame):
webkit-commit-queue
pushed a commit
to hyjorc1/WebKit
that referenced
this pull request
Apr 4, 2025
…rtions https://bugs.webkit.org/show_bug.cgi?id=291090 rdar://148601073 Reviewed by Yusuke Suzuki. This patch enhances loop unrolling debugging by adding support for tracking the original source block (cloneSource) for each cloned basic block in DFGBasicBlock. The cloneSource pointer is enabled only under ASSERT_ENABLED to avoid runtime overhead in release builds. Additionally, Graph::dumpBlockHeader() now prints the clone source block if available, improving traceability during debugging and validation of loop unrolling logic. After loop unrolling, we might see output like the following, where block 17 is a clone of block 9: 9 24: Block WebKit#9 (bc#52): 0 9 24: D@62:<!0:-> LoopHint(MustGen, W:SideState, bc#52, ExitValid) ... 17 24: Block WebKit#17<-WebKit#9 (bc#52): 0 17 24: D@256:<!0:-> LoopHint(MustGen, W:SideState, bc#52, ExitValid) ... * Source/JavaScriptCore/dfg/DFGBasicBlock.h: * Source/JavaScriptCore/dfg/DFGGraph.cpp: (JSC::DFG::Graph::dumpBlockHeader): * Source/JavaScriptCore/dfg/DFGLoopUnrollingPhase.cpp: (JSC::DFG::LoopUnrollingPhase::unrollLoop): Canonical link: https://commits.webkit.org/293265@main
webkit-commit-queue
pushed a commit
that referenced
this pull request
Jul 23, 2026
…s' per-flex-item static helpers https://bugs.webkit.org/show_bug.cgi?id=320019 Reviewed by Antti Koivisto. The per-item static helpers in FlexFormattingUtils took the flex container as an explicit (const RenderFlexibleBox&, const RenderBox& flexItem) pair, even though a flex item's parent is always the flex container. Drop the container argument from these twelve statics and derive it internally from the item's parent, mirroring what ScopedCrossAxisOverrideForFlexItem already does: auto& flexBox = downcast<RenderFlexibleBox>(*flexItem.parent()); The two that only forwarded the container to other statics (useContentBasedMinimumSize, flexBasisForFlexItem) no longer need it at all. All callers drop the container argument: the utils' own static-to-static calls and instance forwarders, RenderFlexibleBox's proxies and ScopedCrossAxisOverrideForFlexItem, and LayoutIntegration::FlexLayout (its positioned flex items are direct out-of-flow children, so their parent is the container too). The container-only statics (isColumnFlow, isHorizontalFlow, computeGap, ...) still take it. Also correct the cross-axis step comments in FlexFormattingContext::layout(): they labelled a section "9.6 (#13 - #16)" although the block also runs the §9.4 steps (#9, dependency around the container's used-cross-size finalization (#15), not by spec number, so the comment now says so. No change in behavior. * Source/WebCore/layout/formattingContexts/flex/FlexFormattingContext.cpp: (WebCore::FlexFormattingContext::layout): * Source/WebCore/layout/formattingContexts/flex/FlexFormattingUtils.cpp: (WebCore::FlexFormattingUtils::crossAxisMarginExtentForFlexItem): (WebCore::FlexFormattingUtils::preferredMainSizeLengthForFlexItem): (WebCore::FlexFormattingUtils::minMainSizeLengthForFlexItem): (WebCore::FlexFormattingUtils::preferredCrossSizeLengthForFlexItem): (WebCore::FlexFormattingUtils::mainAxisOverflowForFlexItem): (WebCore::FlexFormattingUtils::hasAutoMarginsInCrossAxis): (WebCore::FlexFormattingUtils::useContentBasedMinimumSize): (WebCore::FlexFormattingUtils::preferredAspectRatioForFlexItem const): (WebCore::FlexFormattingUtils::needToStretchFlexItemLogicalHeight const): (WebCore::FlexFormattingUtils::innerCrossSizeForFlexItem): (WebCore::FlexFormattingUtils::availableAlignmentSpaceForFlexItem const): (WebCore::FlexFormattingUtils::marginBoxAscentForFlexItem const): (WebCore::FlexFormattingUtils::mainAxisIsFlexItemInlineAxis): (WebCore::FlexFormattingUtils::flexBasisForFlexItem): (WebCore::FlexFormattingUtils::alignmentForFlexItem): (WebCore::FlexFormattingUtils::hasDefiniteCrossSizeForFlexItem): (WebCore::FlexFormattingUtils::crossAxisMarginExtentForFlexItem const): (WebCore::FlexFormattingUtils::preferredMainSizeLengthForFlexItem const): (WebCore::FlexFormattingUtils::minMainSizeLengthForFlexItem const): (WebCore::FlexFormattingUtils::preferredCrossSizeLengthForFlexItem const): (WebCore::FlexFormattingUtils::hasAutoMarginsInCrossAxis const): (WebCore::FlexFormattingUtils::useContentBasedMinimumSize const): (WebCore::FlexFormattingUtils::innerCrossSizeForFlexItem const): (WebCore::FlexFormattingUtils::mainAxisIsFlexItemInlineAxis const): (WebCore::FlexFormattingUtils::flexBasisForFlexItem const): (WebCore::FlexFormattingUtils::alignmentForFlexItem const): (WebCore::FlexFormattingUtils::hasDefiniteCrossSizeForFlexItem const): * Source/WebCore/layout/formattingContexts/flex/FlexFormattingUtils.h: * Source/WebCore/layout/integration/flex/LayoutIntegrationFlexLayout.cpp: (WebCore::LayoutIntegration::FlexLayout::firstLineBaseline const): (WebCore::LayoutIntegration::FlexLayout::lastLineBaseline const): (WebCore::LayoutIntegration::FlexLayout::baselineFlexItemInLine const): (WebCore::LayoutIntegration::FlexLayout::staticCrossAxisPositionForPositionedFlexItem): * Source/WebCore/rendering/RenderFlexibleBox.cpp: (WebCore::RenderFlexibleBox::ScopedCrossAxisOverrideForFlexItem::ScopedCrossAxisOverrideForFlexItem): (WebCore::RenderFlexibleBox::canUseFlexItemForPercentageResolution): (WebCore::RenderFlexibleBox::computeBlockAxisContentSizeForFlexItem): (WebCore::RenderFlexibleBox::setOverridingMainSizeForFlexItem): (WebCore::RenderFlexibleBox::resetAutoMarginsAndLogicalTopInCrossAxis): (WebCore::RenderFlexibleBox::useContentBasedMinimumBlockSize const): (WebCore::RenderFlexibleBox::hasStretchedFlexItemWithAspectRatio const): (WebCore::RenderFlexibleBox::mainAxisIsFlexItemInlineAxis const): (WebCore::RenderFlexibleBox::flexBasisForFlexItem const): (WebCore::RenderFlexibleBox::alignmentForFlexItem const): (WebCore::RenderFlexibleBox::hasDefiniteCrossSizeForFlexItem const): (WebCore::RenderFlexibleBox::flexItemMainSizeIsDefinite): Canonical link: https://commits.webkit.org/317776@main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
5ae7ce2