[JSC] Assert Operation and HostFunction are in JITOperationsList
Created attachment 410898 [details] Patch
Created attachment 410913 [details] Patch
Created attachment 410922 [details] Patch
Created attachment 410954 [details] Patch
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 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 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.
Committed r268316: <https://trac.webkit.org/changeset/268316>
<rdar://problem/70177850>
Committed r268317: <https://trac.webkit.org/changeset/268317>
Committed r268325: <https://trac.webkit.org/changeset/268325>