Skip to content

Conversation

@alancoon
Copy link
Owner

shrug emoji

@alancoon alancoon self-assigned this Sep 23, 2022
alancoon pushed a commit that referenced this pull request Dec 9, 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: [ #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
alancoon pushed a commit that referenced this pull request Jan 12, 2023
https://bugs.webkit.org/show_bug.cgi?id=249765
rdar://103631099

Reviewed by Mark Lam.

In ARM64, we are leveraging LDR style address, which can take 32bit index in addressing and zero-extend / sign-extend that in load/store.
This is useful since WasmAddress' index is 32bit and we need to zero-extend it. However, we cannot use this addressing when there is an
offset since this addressing cannot encode offset. As a result, we are emitting Move32 and Add64 when there is an offset.
However, ARM64 can do even better for that case since ARM64 add / sub instructions also support LDR style extension.

This patch adds AddZeroExtend64 and AddSignExtend64. They take 32bit second operand and extend it before adding. This is particularly useful
when computing WasmAddress. We also leverage this in AirIRGenerator.

In the added testb3, the generated code is changed as follows.

    Before:
        O2: testWasmAddressWithOffset()...
        Generated JIT code for Compilation:
            Code at [0x115f74980, 0x115f749a0):
                     <0> 0x115f74980:    pacibsp
                     <4> 0x115f74984:    stp      fp, lr, [sp, #-16]!
                     <8> 0x115f74988:    mov      fp, sp
                    <12> 0x115f7498c:    ubfx     x0, x0, #0, WebKit#32; emitSave
                    <16> 0x115f74990:    add      x0, x2, x0
                    <20> 0x115f74994:    sturb    w1, [x0, #1]
                    <24> 0x115f74998:    ldp      fp, lr, [sp], WebKit#16
                    <28> 0x115f7499c:    retab

    After:
        O2: testWasmAddressWithOffset()...
        Generated JIT code for Compilation:
            Code at [0x121108980, 0x1211089a0):
                     <0> 0x121108980:    pacibsp
                     <4> 0x121108984:    stp      fp, lr, [sp, #-16]!
                     <8> 0x121108988:    mov      fp, sp
                    <12> 0x12110898c:    add      x0, x2, w0, uxtw; emitSave
                    <16> 0x121108990:    sturb    w1, [x0, #1]
                    <20> 0x121108994:    ldp      fp, lr, [sp], WebKit#16
                    <24> 0x121108998:    retab

* Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:
(JSC::MacroAssemblerARM64::addZeroExtend64):
(JSC::MacroAssemblerARM64::addSignExtend64):
* Source/JavaScriptCore/b3/B3LowerToAir.cpp:
* Source/JavaScriptCore/b3/air/AirInstInlines.h:
(JSC::B3::Air::isAddZeroExtend64Valid):
(JSC::B3::Air::isAddSignExtend64Valid):
* Source/JavaScriptCore/b3/air/AirOpcode.opcodes:

Canonical link: https://commits.webkit.org/258259@main
alancoon pushed a commit that referenced this pull request Jan 12, 2023
https://bugs.webkit.org/show_bug.cgi?id=250196
rdar://98798050

Reviewed by Simon Fraser and Dean Jackson.

WebKit has long accidentally depended on the combination of two somewhat
unusual behavioral quirks in CGIOSurfaceContext:

1) (Source) If you make a CGImageRef from one CGIOSurfaceContext via
CGIOSurfaceContextCreateImage, and mutate the original IOSurface under the hood
(or in a different process) in a way that CGIOSurfaceContext does not know,
CGIOSurfaceContextCreateImage will return the same CGImageRef when called later.

2) (Destination) If you make a CGImageRef from one CGIOSurfaceContext via
CGIOSurfaceContextCreateImage, paint it into a different CGIOSurfaceContext,
then mutate the original IOSurface, and paint the same CGImageRef again,
the updated IOSurface contents will be used the second time.

The second quirk has never worked with unaccelerated CoreGraphics bitmap context
destinations. Instead, in the unaccelerated case, the CGImageRef acts as a
snapshot of the surface at the time it was created.

We've long had code to handle this, forcing CGIOSurfaceContextCreateImage to
re-create the CGImageRef each time we paint it (by drawing an empty rect into
the CGIOSurfaceContext), working around quirk #1 and thus bypassing quirk WebKit#2,
if we're painting into an unaccelerated backing store.

It turns out our CG display list backing store implementation behaves like a
CG bitmap context (without quirk WebKit#2), and so currently any IOSurfaces painted into
CG display list backing store from a CGImageRef created by CGIOSurfaceContextCreateImage
(but not -CreateImageReference) become stale if painted multiple times.

To avoid this, extend the workaround to apply to any destination context that
claims that it needs the workaround, and use it whenever painting an IOSurface
into anything other than a CGIOSurfaceContext.

* Source/WebCore/platform/graphics/BifurcatedGraphicsContext.cpp:
(WebCore::BifurcatedGraphicsContext::needsCachedNativeImageInvalidationWorkaround):
* Source/WebCore/platform/graphics/BifurcatedGraphicsContext.h:
Make BifurcatedGraphicsContext assume the more conservative mode of its two children.

* Source/WebCore/platform/graphics/GraphicsContext.h:
(WebCore::GraphicsContext::needsCachedNativeImageInvalidationWorkaround):
Assume that by default, GraphicsContexts need the workaround.

* Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:
(WebCore::GraphicsContextCG::needsCachedNativeImageInvalidationWorkaround):
* Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:
GraphicsContextCG needs the workaround, except in the IOSurface->IOSurface case.

* Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:
(WebCore::ImageBufferIOSurfaceBackend::finalizeDrawIntoContext):
Confer with the GraphicsContext about its need for the workaround
instead of hardcoding the behavior here.

* Source/WebKit/Shared/RemoteLayerTree/CGDisplayListImageBufferBackend.mm:
CG display list graphics contexts need the workaround.

Canonical link: https://commits.webkit.org/258586@main
alancoon pushed a commit that referenced this pull request Jan 30, 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 #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants