Bug 217500 - [JSC] Assert Operation and HostFunction are in JITOperationsList
Summary: [JSC] Assert Operation and HostFunction are in JITOperationsList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-08 17:43 PDT by Yusuke Suzuki
Modified: 2020-10-11 14:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (46.19 KB, patch)
2020-10-08 17:54 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (306.14 KB, patch)
2020-10-08 22:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (312.31 KB, patch)
2020-10-09 00:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (313.68 KB, patch)
2020-10-09 11:21 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-10-08 17:43:58 PDT
[JSC] Assert Operation and HostFunction are in JITOperationsList
Comment 1 Yusuke Suzuki 2020-10-08 17:54:55 PDT
Created attachment 410898 [details]
Patch
Comment 2 Yusuke Suzuki 2020-10-08 22:14:26 PDT
Created attachment 410913 [details]
Patch
Comment 3 Yusuke Suzuki 2020-10-09 00:50:46 PDT
Created attachment 410922 [details]
Patch
Comment 4 Yusuke Suzuki 2020-10-09 11:21:16 PDT
Created attachment 410954 [details]
Patch
Comment 5 Saam Barati 2020-10-09 11:32:30 PDT
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?
Comment 6 Mark Lam 2020-10-09 13:05:31 PDT
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.
Comment 7 Yusuke Suzuki 2020-10-10 22:26:07 PDT
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.
Comment 8 Yusuke Suzuki 2020-10-10 22:27:33 PDT
Committed r268316: <https://trac.webkit.org/changeset/268316>
Comment 9 Radar WebKit Bug Importer 2020-10-10 22:28:17 PDT
<rdar://problem/70177850>
Comment 10 Yusuke Suzuki 2020-10-11 01:48:58 PDT
Committed r268317: <https://trac.webkit.org/changeset/268317>
Comment 11 Yusuke Suzuki 2020-10-11 14:54:56 PDT
Committed r268325: <https://trac.webkit.org/changeset/268325>