Bug 184702 – Templatize CodePtr/Refs/FunctionPtrs with PtrTags.

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

[Mark Lam]

<rdar://problem/35391681>

[Mark Lam]

Created attachment 338151 [details]
proposed patch.

[EWS Watchlist]

Attachment 338151 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:355:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:221:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLSlowPathCallKey.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 187 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 338152 [details]
proposed patch rebased.

[EWS Watchlist]

Attachment 338152 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:355:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:221:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLSlowPathCallKey.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 187 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 338152 [details]
proposed patch rebased.

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

r=me.  The suggestions about using alternate tags can be addressed in another bug.

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:332
> +    CodeLocationLabel<JSEntryPtrTag> m_callReturnLocationOrPatchableJump;
> +    CodeLocationLabel<JSEntryPtrTag> m_hotPathBeginOrSlowPathStart;
> +    CodeLocationNearCall<JSEntryPtrTag> m_hotPathOther;

I think these should be JSInternalPtrTag, since these are not being used as targets for a JS call.  They are being used to point at some patchable stuff in the call IC.

> Source/JavaScriptCore/ftl/FTLLazySlowPath.h:88
> +    CodeLocationLabel<JSEntryPtrTag> m_done;

I think this should be JSInternalPtrTag.  It's not meant to point to the call entrypoint of anything.

> Source/JavaScriptCore/jit/JITMathIC.h:67
> +    CodeLocationLabel<JSEntryPtrTag> doneLocation() { return m_inlineStart.labelAtOffset(m_inlineSize); }
> +    CodeLocationLabel<JSEntryPtrTag> slowPathStartLocation() { return m_inlineStart.labelAtOffset(m_deltaFromStartToSlowPathStart); }
> +    CodeLocationCall<JSEntryPtrTag> slowPathCallLocation() { return m_inlineStart.callAtOffset(m_deltaFromStartToSlowPathCallLocation); }

I think these should be JSInternal, since none of these are meant to call to the JS call entrypoint of any function.

> Source/JavaScriptCore/jit/JITMathIC.h:250
> +    CodeLocationLabel<JSEntryPtrTag> m_inlineStart;

ISInternal, because this does not for doing JS calls

> Source/JavaScriptCore/llint/LLIntData.cpp:78
> +        JSEntryPtrTag, // llint_program_prologue
> +        JSEntryPtrTag, // llint_eval_prologue
> +        JSEntryPtrTag, // llint_module_program_prologue
> +        JSEntryPtrTag, // llint_function_for_call_prologue
> +        JSEntryPtrTag, // llint_function_for_construct_prologue
> +        JSEntryPtrTag, // llint_function_for_call_arity_check
> +        JSEntryPtrTag, // llint_function_for_construct_arity_check
> +        JSEntryPtrTag, // llint_generic_return_point
>          BytecodePtrTag, // llint_throw_from_slow_path_trampoline
>          ExceptionHandlerPtrTag, // llint_throw_during_call_trampoline
> -        CodePtrTag, // llint_native_call_trampoline
> -        CodePtrTag, // llint_native_construct_trampoline
> -        CodePtrTag, // llint_internal_function_call_trampoline
> -        CodePtrTag, // llint_internal_function_construct_trampoline
> +        JSEntryPtrTag, // llint_native_call_trampoline
> +        JSEntryPtrTag, // llint_native_construct_trampoline
> +        JSEntryPtrTag, // llint_internal_function_call_trampoline
> +        JSEntryPtrTag, // llint_internal_function_construct_trampoline

It would be cool if we could give these JSInternal since these aren't meant to be used as targets of JS calls.

[Mark Lam]

Created attachment 338157 [details]
patch for landing.

[EWS Watchlist]

Attachment 338157 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:355:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:221:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLSlowPathCallKey.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 187 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 338170 [details]
patch for landing + speculative build fix for Windows.

[EWS Watchlist]

Attachment 338170 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:355:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:221:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLSlowPathCallKey.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 187 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 338179 [details]
patch for landing + speculative build fix for Windows.

[EWS Watchlist]

Attachment 338179 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:355:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:221:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLSlowPathCallKey.h:56:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 187 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 338179 [details]
patch for landing + speculative build fix for Windows.

Attachment 338179 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7351388

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.no-cjit-collect-continuously

[Mark Lam]

(In reply to Build Bot from comment #12)
> Comment on attachment 338179 [details]
> patch for landing + speculative build fix for Windows.
> 
> Attachment 338179 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/7351388
> 
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.no-cjit-
> collect-continuously

I'm quite sure that this is not caused by my patch.  I also cannot reproduce it locally.  Will land shortly.

[Mark Lam]

Landed in r230748: <http://trac.webkit.org/r230748>.

[Fujii Hironori]

WinCairo builds get broken. Bug 184725

[Tomas Popela]

I pushed a build fix for ARM in https://trac.webkit.org/r230762

[Tomas Popela]

And also one small for GCC - https://trac.webkit.org/r230761 (that was triggered by our hardening flags in Fedora)

[Tomas Popela]

And another build failure on ARM - bug 184730