Bug 217362 – [JSC] More consistent PtrTagging for code types

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

[Yusuke Suzuki]

[JSC] More consistent PtrTagging for code types

[Yusuke Suzuki]

Created attachment 410608 [details]
Patch

[Yusuke Suzuki]

Created attachment 410612 [details]
Patch

[Mark Lam]

Comment on attachment 410612 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/llint/LLIntExceptions.cpp:76
> +MacroAssemblerCodeRef<ExceptionHandlerPtrTag> catcher(OpcodeSize size)

Can we call this handleCatch (verb) instead of catcher (noun)?

> Source/JavaScriptCore/llint/LLIntExceptions.cpp:80
> +        return catcherThunk(size);

Ditto: handleCatchThunk?

> Source/JavaScriptCore/llint/LLIntThunks.cpp:152
> +            codeRef.construct(generateThunkWithJumpTo<JITThunkPtrTag>(wasm_function_prologue, "function for call"));

Is there any reason this shouldn't also be using JSEntryPtrTag (and obsoleting JITThunkPtrTag)?  Looks like this is the last use of it.  We used to use JITThunkPtrTag for certain pointers based on where they point to.  Now, you've changed this to based on where the pointers will be used.  Based on where the pointer is used, it sounds like wasm_function_prologue should be using JSEntryPtrTag too.

You don't have to do this in this patch, but maybe it warrants a look later.

[Yusuke Suzuki]

Comment on attachment 410612 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/llint/LLIntExceptions.cpp:76
>> +MacroAssemblerCodeRef<ExceptionHandlerPtrTag> catcher(OpcodeSize size)
> 
> Can we call this handleCatch (verb) instead of catcher (noun)?

Changed.

>> Source/JavaScriptCore/llint/LLIntExceptions.cpp:80
>> +        return catcherThunk(size);
> 
> Ditto: handleCatchThunk?

Changed.

>> Source/JavaScriptCore/llint/LLIntThunks.cpp:152
>> +            codeRef.construct(generateThunkWithJumpTo<JITThunkPtrTag>(wasm_function_prologue, "function for call"));
> 
> Is there any reason this shouldn't also be using JSEntryPtrTag (and obsoleting JITThunkPtrTag)?  Looks like this is the last use of it.  We used to use JITThunkPtrTag for certain pointers based on where they point to.  Now, you've changed this to based on where the pointers will be used.  Based on where the pointer is used, it sounds like wasm_function_prologue should be using JSEntryPtrTag too.
> 
> You don't have to do this in this patch, but maybe it warrants a look later.

wasmFunctionEntryThunk's code is used for jump target. So this I not JSEntry. I think JITThunkPtrTag is the right tag for this thunk.

[Yusuke Suzuki]

Committed r268077: <https://trac.webkit.org/changeset/268077>

[Radar WebKit Bug Importer]

<rdar://problem/70019125>