Bug 122306 – Add callOperation to Baseline JIT

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

Add callOperation framework to baseline JIT similar to what exists in DFG.

[Michael Saboff]

Created attachment 213313 [details]
Patch

Converted cti_op_new_regexp() use to callOperation(operationNewRegexp()) as an example use.

[Mark Lam]

Comment on attachment 213313 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOperations.h:176
> +void JIT_OPERATION operationStackCheck(ExecState*, void*) WTF_INTERNAL;

This line is not pertinent to this patch, is it?

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 213313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213313&action=review
> 
> > Source/JavaScriptCore/jit/JITOperations.h:176
> > +void JIT_OPERATION operationStackCheck(ExecState*, void*) WTF_INTERNAL;
> 
> This line is not pertinent to this patch, is it?

I'll remove that.  It was part of something else I was trying.

[Geoffrey Garen]

Comment on attachment 213313 [details]
Patch

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

> Source/JavaScriptCore/jit/JITInlines.h:220
> +ALWAYS_INLINE MacroAssembler::Call JIT::appendCallWithExceptionCheckSetResult(const FunctionPtr& function, int dst)
> +{
> +    MacroAssembler::Call call = appendCallWithExceptionCheck(function);
> +    emitStoreCell(dst, returnValueRegister);
> +    return call;
> +}

This doesn't look right. The return value from a stub is not guaranteed to be a cell. This will break when we start migrating more functions.

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 213313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213313&action=review
> 
> > Source/JavaScriptCore/jit/JITInlines.h:220
> > +ALWAYS_INLINE MacroAssembler::Call JIT::appendCallWithExceptionCheckSetResult(const FunctionPtr& function, int dst)
> > +{
> > +    MacroAssembler::Call call = appendCallWithExceptionCheck(function);
> > +    emitStoreCell(dst, returnValueRegister);
> > +    return call;
> > +}
> 
> This doesn't look right. The return value from a stub is not guaranteed to be a cell. This will break when we start migrating more functions.

I'll make this version's name Cell specific.

[Michael Saboff]

Created attachment 213334 [details]
Patch with Updates from reviews

[Geoffrey Garen]

Comment on attachment 213334 [details]
Patch with Updates from reviews

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

> Source/JavaScriptCore/jit/JITInlines.h:232
> +ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(J_JITOperation_E operation, int dst)
> +{
> +    setupArgumentsExecState();
> +    return appendCallWithExceptionCheckSetCellResult(operation, dst);
> +}
> +
> +ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(J_JITOperation_EP operation, int dst, void* pointer)
> +{
> +    setupArgumentsWithExecState(TrustedImmPtr(pointer));
> +    return appendCallWithExceptionCheckSetCellResult(operation, dst);
> +}

I don't think you understood my meaning. The function types here are J_JITOperation_E and J_JITOperation_EP:

typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_E)(ExecState*);
typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_EP)(ExecState*, void*);

It's inappropriate for a utility function that calls a function returning EncodedJSValue to assume that the return value is a cell. That's the assembly equivalent of an invalid cast.

You should either change the return type of operationNewRegexp to be JSCell*, or you should change these call sites to store a full EncodedJSValue.

[Michael Saboff]

Created attachment 213372 [details]
Updated patch

[Geoffrey Garen]

Comment on attachment 213372 [details]
Updated patch

r=me

[Michael Saboff]

Committed r156896: <http://trac.webkit.org/changeset/156896>