Bug 217500 – [JSC] Assert Operation and HostFunction are in JITOperationsList

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Assert Operation and HostFunction are in JITOperationsList

[Yusuke Suzuki]

Created attachment 410898 [details]
Patch

[Yusuke Suzuki]

Created attachment 410913 [details]
Patch

[Yusuke Suzuki]

Created attachment 410922 [details]
Patch

[Yusuke Suzuki]

Created attachment 410954 [details]
Patch

[Saam Barati]

Comment on attachment 410954 [details]
Patch

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

> Source/WTF/wtf/PtrTag.h:52
> +enum class PtrTagCalledFromType : uint8_t { CalledFromNative, CalledFromJIT, NotCalled };

Should we call this PtrTagCallerType?

If not, I don’t think you need to repeat “CalledFrom” in the enum values.

> Source/WTF/wtf/PtrTag.h:53
> +enum class PtrTagTargetType : uint8_t { Native, JIT, };

And this PtrTagCalleeType?

[Mark Lam]

Comment on attachment 410954 [details]
Patch

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

r=me too with some suggested improvements.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:112
> +    ASSERT(calledFromType == PtrTagCalledFromType::CalledFromJIT);

This should be a static_assert.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:117
> +            else if (tag == HostFunctionPtrTag)

This should be a constexpr as well.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:120
> +                ASSERT_NOT_REACHED();

This can be a RELEASE_ASSERT_NOT_REACHED().

Or better yet, add a `static_assert(tag == OperationPtrTag || tag == HostFunctionPtrTag)` above before the `if constexpr (tag == OperationPtrTag)` check, change `else if (tag == HostFunctionPtrTag)` to `else`, and remove this `else ASSERT...` altogether. This way, we can have a compile time check and catch any errors earlier.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:129
> +    ASSERT(calledFromType == PtrTagCalledFromType::CalledFromJIT);

Ditto: make this a static_assert.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:137
> +            else
> +                ASSERT_NOT_REACHED();

Make the same changes as in tagJSCCodePtrImpl() above and remove this.

> Source/JavaScriptCore/runtime/NativeFunction.h:73
> +        : m_ptr(tagCFunctionPtr<void*, HostFunctionPtrTag>(func.m_ptr))

Should we rename HostFunctionPtrTag to NativeFunctionPtrTag to be consistent with the TaggedNativeFunction name?  On the other hand, I like having the distinction that a HostFunction is a native function that is called like a JS function, where as a native function may not necessarily be a HostFunction (e.g. can be an Operation).  If you keep HostFunctionPtrTag (and I think this is the better options), I suggest renaming TaggedNativeFunction to TaggedHostFunction (can do this in a later patch).  This TaggedNativeFunction class is only used with HostFunctions, right?

>> Source/WTF/wtf/PtrTag.h:52
>> +enum class PtrTagCalledFromType : uint8_t { CalledFromNative, CalledFromJIT, NotCalled };
> 
> Should we call this PtrTagCallerType?
> 
> If not, I don’t think you need to repeat “CalledFrom” in the enum values.

FWIW, I like Saam's idea of using PtrTagCallerType and PtrTagCalleeType.  How about something like:

    enum class PtrTagCallerType : uint8_t { Native, JIT, None };

> Source/WTF/wtf/PtrTag.h:446
> +inline PtrType tagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag stackPointer)

Let change the stackPointer arg to be of type const void* since this function is explicitly meant for signing with sp anyway.  This way, all the clients (above) don't have to cast sp to PtrTag in order to call this function.

> Source/WTF/wtf/PtrTag.h:453
> +inline PtrType untagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag stackPointer)

Ditto: const void* stackPointer

> Source/WTF/wtf/PtrTag.h:510
> +inline PtrType tagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag) { return ptr; }

Ditto.

> Source/WTF/wtf/PtrTag.h:513
> +inline PtrType untagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag) { return ptr; }

Ditto.

[Yusuke Suzuki]

Comment on attachment 410954 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:112
>> +    ASSERT(calledFromType == PtrTagCalledFromType::CalledFromJIT);
> 
> This should be a static_assert.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:117
>> +            else if (tag == HostFunctionPtrTag)
> 
> This should be a constexpr as well.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:120
>> +                ASSERT_NOT_REACHED();
> 
> This can be a RELEASE_ASSERT_NOT_REACHED().
> 
> Or better yet, add a `static_assert(tag == OperationPtrTag || tag == HostFunctionPtrTag)` above before the `if constexpr (tag == OperationPtrTag)` check, change `else if (tag == HostFunctionPtrTag)` to `else`, and remove this `else ASSERT...` altogether. This way, we can have a compile time check and catch any errors earlier.

Changed to using static_assert.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:129
>> +    ASSERT(calledFromType == PtrTagCalledFromType::CalledFromJIT);
> 
> Ditto: make this a static_assert.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:137
>> +                ASSERT_NOT_REACHED();
> 
> Make the same changes as in tagJSCCodePtrImpl() above and remove this.

Done.

>> Source/JavaScriptCore/runtime/NativeFunction.h:73
>> +        : m_ptr(tagCFunctionPtr<void*, HostFunctionPtrTag>(func.m_ptr))
> 
> Should we rename HostFunctionPtrTag to NativeFunctionPtrTag to be consistent with the TaggedNativeFunction name?  On the other hand, I like having the distinction that a HostFunction is a native function that is called like a JS function, where as a native function may not necessarily be a HostFunction (e.g. can be an Operation).  If you keep HostFunctionPtrTag (and I think this is the better options), I suggest renaming TaggedNativeFunction to TaggedHostFunction (can do this in a later patch).  This TaggedNativeFunction class is only used with HostFunctions, right?

Yeah, I think we should rename TaggedNativeFunction to TaggedHostFunction since it is only used for host function. I'll do that in the later patch.

>>> Source/WTF/wtf/PtrTag.h:52
>>> +enum class PtrTagCalledFromType : uint8_t { CalledFromNative, CalledFromJIT, NotCalled };
>> 
>> Should we call this PtrTagCallerType?
>> 
>> If not, I don’t think you need to repeat “CalledFrom” in the enum values.
> 
> FWIW, I like Saam's idea of using PtrTagCallerType and PtrTagCalleeType.  How about something like:
> 
>     enum class PtrTagCallerType : uint8_t { Native, JIT, None };

CallerType sounds good. Changed

>> Source/WTF/wtf/PtrTag.h:53
>> +enum class PtrTagTargetType : uint8_t { Native, JIT, };
> 
> And this PtrTagCalleeType?

Sounds good, changed.

>> Source/WTF/wtf/PtrTag.h:446
>> +inline PtrType tagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag stackPointer)
> 
> Let change the stackPointer arg to be of type const void* since this function is explicitly meant for signing with sp anyway.  This way, all the clients (above) don't have to cast sp to PtrTag in order to call this function.

Nice, changed.

>> Source/WTF/wtf/PtrTag.h:453
>> +inline PtrType untagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag stackPointer)
> 
> Ditto: const void* stackPointer

Changed.

>> Source/WTF/wtf/PtrTag.h:510
>> +inline PtrType tagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag) { return ptr; }
> 
> Ditto.

Fixed.

>> Source/WTF/wtf/PtrTag.h:513
>> +inline PtrType untagCodePtrWithStackPointerForJITCall(PtrType ptr, PtrTag) { return ptr; }
> 
> Ditto.

Fixed.

[Yusuke Suzuki]

Committed r268316: <https://trac.webkit.org/changeset/268316>

[Radar WebKit Bug Importer]

<rdar://problem/70177850>

[Yusuke Suzuki]

Committed r268317: <https://trac.webkit.org/changeset/268317>

[Yusuke Suzuki]

Committed r268325: <https://trac.webkit.org/changeset/268325>