Bug 229618 – Make ARM64 and X86_64 probe code a little bit more efficient.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

We were using an unnecessary indirect call when we can be using a direct call.

[Radar WebKit Bug Importer]

<rdar://problem/82445743>

[Mark Lam]

Created attachment 436642 [details]
proposed patch.

[Mark Lam]

Created attachment 436643 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 436643 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:18
> +        For MacroAssemblerX86Common.cpp, we left the X86 and MSVC implementations unchanged.
> +        For X86, I don't know the stack alignment requirements (if any) plus we might want
> +        to delete this code eventually since we're not supporting the X86 JIT anymore.

Right.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-323
> -static_assert(JIT_PROBE_EXECUTOR_PTR_TAG == JITProbeExecutorPtrTag);

Let's remove JITProbeExecutorPtrTag from JSCPtrTag.h since it is no longer used.

[Mark Lam]

(In reply to Yusuke Suzuki from comment #4)
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-323
> > -static_assert(JIT_PROBE_EXECUTOR_PTR_TAG == JITProbeExecutorPtrTag);
> 
> Let's remove JITProbeExecutorPtrTag from JSCPtrTag.h since it is no longer
> used.

Ah yes, I had planned to do that but forgot.

[Mark Lam]

Created attachment 436647 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 436647 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r281718: <http://trac.webkit.org/r281718>.

[Mark Lam]

This patch has a bug (accidentally deleted 2 lines) that is fixed in https://bugs.webkit.org/show_bug.cgi?id=229629.

[Diego Pino]

This patch introduced a regression in Ubuntu 18.04. Compilation aborts on a linking error:

https://build.webkit.org/#/builders/71/builds/222

FAILED: lib/libjavascriptcoregtk-4.0.so.18.19.2
: && /usr/lib/ccache/g++-8 -fPIC -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wun
/usr/bin/ld.gold: error: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp.o: requires dynamic R_X86_64_PC32 reloc again
collect2: error: ld returned 1 exit status
[207/6083] Building CXX object Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/RewriteAtomicFunctionExpressions.cpp.o
ninja: build stopped: subcommand failed.
program finished with exit code 254

This Ubuntu 18.04 bot uses GCC-8 (v8.4.0) and links with GNU Gold (GNU Binutils for Ubuntu 2.30 - 1.15).

On the Debian bot this regression didn't happen. It uses a similar compiler and linker. Compiler: v8.3.0. Linker: GNU Binutils for Debian 2.31.1 - 1.16.

Any clue or hint on what might have gone wrong?
There is a Debian bot with

[Mark Lam]

(In reply to Diego Pino from comment #10)
> This patch introduced a regression in Ubuntu 18.04. Compilation aborts on a
> linking error:
> 
...
> /usr/bin/ld.gold: error:
> Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/
> DerivedSources/unified-sources/UnifiedSource-cd2e8cfa-2.cpp.o: requires
> dynamic R_X86_64_PC32 reloc again
> collect2: error: ld returned 1 exit status
...
> Any clue or hint on what might have gone wrong?
> There is a Debian bot with

Looks like your linker doesn't support linking the direct branch I used in the inline asm statement in ctiMasmProbeTrampoline.  I can try re-instating the old way of working for non-darwin ports since the diff isn't big in for X86_64.

[Mark Lam]

(In reply to Mark Lam from comment #11)
> Looks like your linker doesn't support linking the direct branch I used in
> the inline asm statement in ctiMasmProbeTrampoline.  I can try re-instating
> the old way of working for non-darwin ports since the diff isn't big in for
> X86_64.

See https://bugs.webkit.org/show_bug.cgi?id=229794.

[Jim Mason]

FYI, also getting an ld fatal error with the Sun linker.  It bails on the new X86_64 inline assembly instruction `call executeJSCJITProbe` (text relocation remains against symbol executeJSCJITProbe.)

If I change to `call executeJSCJITProbe@PLT`, it links and runs successfully.  I am patching it locally like this:

--- a/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp       2021-09-01 18:03:12.777668266 +0000
+++ b/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp       2021-09-01 19:00:37.913407269 +0000
@@ -596,7 +596,7 @@
     "movq %xmm15, " STRINGIZE_VALUE_OF(PROBE_CPU_XMM15_OFFSET) "(%rsp)" "\n"
 
     "movq %rsp, %rdi" "\n" // the Probe::State* arg.
-    "call " SYMBOL_STRING(executeJSCJITProbe) "\n"
+    "call " GLOBAL_REFERENCE(executeJSCJITProbe) "\n"
 
     // Make sure the Probe::State is entirely below the result stack pointer so
     // that register values are still preserved when we call the initializeStack

where the GLOBAL_REFERENCE macro is appending @PLT.

I may be the same linker issue Diego is reporting in Ubuntu.

[Jim Mason]

I applied Yusuke's speculative patch (r281911) instead of my own, and can confirm it clears the issue with the Sun linker.