Bug 212561 - AssemblyHelpers::callExceptionFuzz() is passing a wrong argument to operationExceptionFuzz().
Summary: AssemblyHelpers::callExceptionFuzz() is passing a wrong argument to operation...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 212556
  Show dependency treegraph
 
Reported: 2020-05-29 21:08 PDT by Mark Lam
Modified: 2020-05-30 14:10 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (17.24 KB, patch)
2020-05-29 23:54 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (4.28 KB, patch)
2020-05-30 00:57 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing. (4.54 KB, patch)
2020-05-30 09:01 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-05-29 21:08:00 PDT
operationExceptionFuzz() expects a JSGlobalObject*.  AssemblyHelpers::callExceptionFuzz() is passing an ExecState* to it.  The FTL implementation is passing a globalObject as expected.
Comment 1 Mark Lam 2020-05-29 23:54:10 PDT
Created attachment 400652 [details]
proposed patch.
Comment 2 Mark Lam 2020-05-29 23:56:06 PDT
Comment on attachment 400652 [details]
proposed patch.

Patch attached to wrong bug.
Comment 3 Mark Lam 2020-05-30 00:57:32 PDT
Created attachment 400659 [details]
proposed patch.
Comment 4 Yusuke Suzuki 2020-05-30 01:06:32 PDT
Comment on attachment 400659 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=400659&action=review

r=me with comments.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:255
> +    move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR1);

This is not necessary if prepareCallOperation is called. Let's just pass VM pointer.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:257
>      prepareCallOperation(vm);

Since we are using prepareCallOperation, you can get CallFrame* via `CallFrame* callFrame = DECLARE_CALL_FRAME(vm);` as it is done in operationExceptionFuzz.
Let's define operationExceptionFuzzWithVM(VM* vmPointer), and implement it as,

operationExceptionFuzzWith(VM* vmPointer)
{
    VM& vm = *vmPointer;
    CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
    JITOperationPrologueCallFrameTracer tracer(vm, callFrame);  /* Note that this frame tracer must be in the direct caller function from JIT code. */
    ...
}

> Source/JavaScriptCore/jit/JITOperations.cpp:2644
> +    operationExceptionFuzz(callFrame->lexicalGlobalObject(*vm));

Can you factor out the code of operationExceptionFuzz and share it with operationExceptionFuzz and operationExceptionFuzzWithCallFrame? JIT_OPERATION functions cannot be called from non JIT code (in particular if JITOperationPrologueCallFrameTracer is used).
Comment 5 Mark Lam 2020-05-30 08:55:37 PDT
Thanks for the review.

(In reply to Yusuke Suzuki from comment #4)
> > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:255
> > +    move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR1);
> 
> This is not necessary if prepareCallOperation is called. Let's just pass VM
> pointer.

Fixed.

> > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:257
> >      prepareCallOperation(vm);
> 
> Since we are using prepareCallOperation, you can get CallFrame* via
> `CallFrame* callFrame = DECLARE_CALL_FRAME(vm);` as it is done in
> operationExceptionFuzz.
> Let's define operationExceptionFuzzWithVM(VM* vmPointer), and implement it
> as,
> 
> operationExceptionFuzzWith(VM* vmPointer)
> {
>     VM& vm = *vmPointer;
>     CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
>     JITOperationPrologueCallFrameTracer tracer(vm, callFrame);  /* Note that
> this frame tracer must be in the direct caller function from JIT code. */
>     ...
> }

Fixed.

> > Source/JavaScriptCore/jit/JITOperations.cpp:2644
> > +    operationExceptionFuzz(callFrame->lexicalGlobalObject(*vm));
> 
> Can you factor out the code of operationExceptionFuzz and share it with
> operationExceptionFuzz and operationExceptionFuzzWithCallFrame?
> JIT_OPERATION functions cannot be called from non JIT code (in particular if
> JITOperationPrologueCallFrameTracer is used).

Since the common part is just a few lines, and the interesting line in there involves calling __builtin_return_address(0), the common part needs to be completely inlined.  I'm not sure what the behavior of __builtin_return_address() is from an inlined function.  Also, on debug builds, inlining is turned off, which breaks this code.

The alternative is to macroize the common part or just copy-paste.  Since the lines are really short, and the effective part that matters is just 2 lines, there isn't a lot of benefit to macroizing this.  I'm going with copy-paste since it makes the code more readable and easier to step through with a debugger.
Comment 6 Mark Lam 2020-05-30 09:01:12 PDT
Created attachment 400669 [details]
patch for landing.
Comment 7 EWS 2020-05-30 14:09:37 PDT
Committed r262351: <https://trac.webkit.org/changeset/262351>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400669 [details].
Comment 8 Radar WebKit Bug Importer 2020-05-30 14:10:17 PDT
<rdar://problem/63794162>