Bug 218143 – [JSC] Add JITCage support

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Add JITCage support

[Yusuke Suzuki]

Created attachment 412218 [details]
Patch

[Yusuke Suzuki]

Created attachment 412222 [details]
Patch

[Yusuke Suzuki]

Created attachment 412223 [details]
Patch

[Yusuke Suzuki]

Created attachment 412236 [details]
Patch

[Yusuke Suzuki]

Created attachment 412483 [details]
Patch

[Yusuke Suzuki]

Created attachment 412500 [details]
Patch

[Caio Lima]

Comment on attachment 412500 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1102
> +        size(labelNarrow, labelWide16, labelWide32, macro (gen) gen() end)

This is one of the causes of crashes on ARMv7 and MIPS. When we have global label definition, we might generate a set of instructions that might clobber normal execution. This happens because of `deferNextLabelAction`. One example of broken code on ARMv7 is below:

```
...
"\tadds sp, #8\n"                                        // /home/igalia/clima/webkit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1119

  OFFLINE_ASM_LOCAL_LABEL(_offlineasm_commonCallOp__llintOpWithMetadata__llintOpWithReturn__llintOp__commonOp__fn__fn__makeReturn__fn__fn__fn__callHelper__slowPathForCall__callCallSlowPath__564_action__dontUpdateSP)
#if OS(DARWIN)
    "\t.section __DATA,__nl_symbol_ptr,non_lazy_symbol_pointers\n"
    "\t.p2align 2\n"
    "\tL" LOCAL_REFERENCE(g_config) "_1110$non_lazy_ptr:\n"
    "\t.indirect_symbol " LOCAL_REFERENCE(g_config) "\n"
    "\t.long 0\n"
    "\t.text\n"
    "\t.align 4\n"
#elif OS(LINUX)
    "\t" LOCAL_LABEL_STRING(offlineasm_arm_got_1110) ":\n"
    "\t.word _GLOBAL_OFFSET_TABLE_-(" LOCAL_LABEL_STRING(offlineasm_arm_got_offset_1110) "+4)\n"
    "\t.word " LOCAL_REFERENCE(g_config) "(GOT)\n"
#endif

OFFLINE_ASM_GLUE_LABEL(js_trampoline_op_call_slow)
    "\tmovw r12, #44698\n"                                   // /home/igalia/clima/webkit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:259
    "\tblx r0\n"                                             // /home/igalia/clima/webkit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1103
...
```

IIUC, this label is not being used on 32-bits. Since we need to declare them to make it compile, I think we might want to place it on unreachable code with a `crash()` call afterwards.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:727
> +    _js_trampoline_%opcodeName%_untag:

Ditto for this definition here.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:754
> +    _js_trampoline_%opcodeName%_tag:

ditto.

[Yusuke Suzuki]

Make sense!

[Yusuke Suzuki]

Created attachment 412574 [details]
Patch

[Yusuke Suzuki]

Created attachment 412594 [details]
Patch

[Yusuke Suzuki]

Created attachment 412597 [details]
Patch

[Yusuke Suzuki]

Created attachment 412620 [details]
Patch

[Yusuke Suzuki]

Created attachment 412621 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70904668>

[Caio Lima]

Comment on attachment 412621 [details]
Patch

ARMv7 EWS is green! 

Regarding MIPS errors, the problem we have is that changes we've made on https://trac.webkit.org/changeset/252713/webkit are not valid anymore. Since we now reach `*return_location` through JIT trampolines and those trampolines are emitting `farJump(t9)`, it means that it's not possible to reach such locations from `ret`. This way, we should aways be considering `t9` as a possible register when emitting `OFFLINE_ASM_CPLOAD`. The diff to fix MIPS failures should be something like this:

```
diff --git a/Source/JavaScriptCore/offlineasm/mips.rb b/Source/JavaScriptCore/offlineasm/mips.rb
index 41b8b9fa7b9d..13736ad9b76f 100644
--- a/Source/JavaScriptCore/offlineasm/mips.rb
+++ b/Source/JavaScriptCore/offlineasm/mips.rb
@@ -681,17 +681,7 @@ def mipsAddPICCode(list)
         | node |
         myList << node
         if node.is_a? Label
-            # FIXME: [JSC] checkpoint_osr_exit_from_inlined_call_trampoline is a return location
-            # and we should name it properly.
-            # https://bugs.webkit.org/show_bug.cgi?id=208236
-            if node.name =~ /^.*_return_location(?:_(?:wide16|wide32))?$/ or node.name.start_with?("_checkpoint_osr_exit_from_inlined_call_trampoline") or node.name.start_with?("_fuzzer_return_early_from_loop_hint")
-                # We need to have a special case for return location labels because they are always
-                # reached from a `ret` instruction. In this case, we need to proper reconfigure `$gp`
-                # using `$ra` instead of using `$t9`.
-                myList << Instruction.new(node.codeOrigin, "pichdr", [MIPS_RETURN_ADDRESS_REG])
-            else
-                myList << Instruction.new(node.codeOrigin, "pichdr", [MIPS_CALL_REG])
-            end
+            myList << Instruction.new(node.codeOrigin, "pichdr", [MIPS_CALL_REG])
         end
     }
     myList
```

I'd be very happy if we are able to remove such hack from `offlineasm/mips.rb`, this is bothering me for a very long time. I'm running tests locally to see if we will have any remaining failure, but they are going to take a while until it is finished. Probably EWS would finish first.

[Saam Barati]

Comment on attachment 412621 [details]
Patch

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

> Source/JavaScriptCore/assembler/JITOperationList.cpp:59
> +        return;

we don't want to add to the map below?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:134
> +#if ENABLE(JIT_CAGE)

shouldn't this go around the Options branch?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:166
> +#if ENABLE(JIT_CAGE)

ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:219
> +#if ENABLE(JIT_CAGE)

ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:331
> +#if ENABLE(JIT_CAGE)

ditto

> Source/JavaScriptCore/llint/LLIntThunks.cpp:118
> +static MacroAssemblerCodeRef<tag> generateThunkWithJumpToReturnPoint(LLIntCode target, const char *thunkKind)

nit: I'd call this "...ToLLIntReturnPoint"

> Source/JavaScriptCore/llint/LLIntThunks.cpp:351
> +    jit.call(GPRInfo::regT3, tag);

where does regT3 come from? Can we give it a name instead of hard coding it everywhere?

[Saam Barati]

Comment on attachment 412621 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSCPtrTag.h:133
> +            return bitwise_cast<PtrType>(JITOperationList::instance().contains(bitwise_cast<void*>(ptr)));

why does contains return a pointer? Seems like a weird name.

[Mark Lam]

Comment on attachment 412621 [details]
Patch

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

> Source/JavaScriptCore/assembler/JITOperationList.cpp:134
> +#endif

nit: can you add `// ENABLE(JIT_OPERATION_VALIDATION)` here?

> Source/JavaScriptCore/runtime/Gate.h:88
> +static constexpr unsigned numberOfGates = 0 JSC_UTILITY_GATES(JSC_COUNT) JSC_JS_GATE_OPCODES(JSC_OPCODE_COUNT) JSC_WASM_GATE_OPCODES(JSC_OPCODE_COUNT);

nit: I think this would read better if expressed as:
static constexpr unsigned numberOfGates = (JSC_UTILITY_GATES(JSC_COUNT)) + (JSC_JS_GATE_OPCODES(JSC_OPCODE_COUNT)) + (JSC_WASM_GATE_OPCODES(JSC_OPCODE_COUNT));

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:133
>> +            return bitwise_cast<PtrType>(JITOperationList::instance().contains(bitwise_cast<void*>(ptr)));
> 
> why does contains return a pointer? Seems like a weird name.

Yes, lets rename contain to map or decode or get (or something like that).  If there's a need for a bool contains, you can add that as an inline wrapper around this.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:134
> +#endif

You can remove this line ...

> Source/JavaScriptCore/runtime/JSCPtrTag.h:136
> +#if ENABLE(JIT_CAGE)

... and remove this line.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:139
> +#endif

nit: add `// ENABLE(JIT_CAGE)`.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:159
> +#endif

Ditto: You can remove this line ...

> Source/JavaScriptCore/runtime/JSCPtrTag.h:161
> +#if ENABLE(JIT_CAGE)

... and this line.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:167
> +#endif

nit: add `// ENABLE(JIT_CAGE)`.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:182
> +    return ptrauth_sign_unauthenticated(ptr, ptrauth_key_process_dependent_code, bitwise_cast<PtrTag>(stackPointer));

The `bitwise_cast<PtrTag>` around stackPointer is unnecessary.

> Source/JavaScriptCore/runtime/JSCPtrTag.h:201
> +    return __builtin_ptrauth_auth(ptr, ptrauth_key_process_dependent_code, bitwise_cast<PtrTag>(stackPointer));

Ditto: don't need the `bitwise_cast<PtrTag>` around stackPointer.

[Yusuke Suzuki]

Comment on attachment 412621 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:134
>> +#if ENABLE(JIT_CAGE)
> 
> shouldn't this go around the Options branch?

Changed.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:166
>> +#if ENABLE(JIT_CAGE)
> 
> ditto

Changed.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:219
>> +#if ENABLE(JIT_CAGE)
> 
> ditto

Changed.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:331
>> +#if ENABLE(JIT_CAGE)
> 
> ditto

Changed.

>> Source/JavaScriptCore/llint/LLIntThunks.cpp:118
>> +static MacroAssemblerCodeRef<tag> generateThunkWithJumpToReturnPoint(LLIntCode target, const char *thunkKind)
> 
> nit: I'd call this "...ToLLIntReturnPoint"

Changed.

>> Source/JavaScriptCore/llint/LLIntThunks.cpp:351
>> +    jit.call(GPRInfo::regT3, tag);
> 
> where does regT3 come from? Can we give it a name instead of hard coding it everywhere?

This is from JSGate's convention. I think naming it to something rather makes LLInt code difficult to read since LLInt code is very careful about register use. Naming convenient name to LLInt register in LLInt code makes us confused what registers are used and not used.

>> Source/JavaScriptCore/runtime/Gate.h:88
>> +static constexpr unsigned numberOfGates = 0 JSC_UTILITY_GATES(JSC_COUNT) JSC_JS_GATE_OPCODES(JSC_OPCODE_COUNT) JSC_WASM_GATE_OPCODES(JSC_OPCODE_COUNT);
> 
> nit: I think this would read better if expressed as:
> static constexpr unsigned numberOfGates = (JSC_UTILITY_GATES(JSC_COUNT)) + (JSC_JS_GATE_OPCODES(JSC_OPCODE_COUNT)) + (JSC_WASM_GATE_OPCODES(JSC_OPCODE_COUNT));

Sounds good. Changed.

>>> Source/JavaScriptCore/runtime/JSCPtrTag.h:133
>>> +            return bitwise_cast<PtrType>(JITOperationList::instance().contains(bitwise_cast<void*>(ptr)));
>> 
>> why does contains return a pointer? Seems like a weird name.
> 
> Yes, lets rename contain to map or decode or get (or something like that).  If there's a need for a bool contains, you can add that as an inline wrapper around this.

OK, changed to map.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:134
>> +#endif
> 
> You can remove this line ...

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:136
>> +#if ENABLE(JIT_CAGE)
> 
> ... and remove this line.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:139
>> +#endif
> 
> nit: add `// ENABLE(JIT_CAGE)`.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:159
>> +#endif
> 
> Ditto: You can remove this line ...

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:161
>> +#if ENABLE(JIT_CAGE)
> 
> ... and this line.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:167
>> +#endif
> 
> nit: add `// ENABLE(JIT_CAGE)`.

Changed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:182
>> +    return ptrauth_sign_unauthenticated(ptr, ptrauth_key_process_dependent_code, bitwise_cast<PtrTag>(stackPointer));
> 
> The `bitwise_cast<PtrTag>` around stackPointer is unnecessary.

Removed.

>> Source/JavaScriptCore/runtime/JSCPtrTag.h:201
>> +    return __builtin_ptrauth_auth(ptr, ptrauth_key_process_dependent_code, bitwise_cast<PtrTag>(stackPointer));
> 
> Ditto: don't need the `bitwise_cast<PtrTag>` around stackPointer.

Removed.

[Yusuke Suzuki]

Created attachment 413114 [details]
Patch for landing

[Yusuke Suzuki]

Committed r269349: <https://trac.webkit.org/changeset/269349>

[Yusuke Suzuki]

Committed r269353: <https://trac.webkit.org/changeset/269353>

[Michael Catanzaro]

This broke the cloop build:

In file included from ../../Source/JavaScriptCore/runtime/Options.h:28,
                 from ../../Source/JavaScriptCore/assembler/CPU.h:28,
                 from ../../Source/JavaScriptCore/heap/GCMemoryOperations.h:28,
                 from ../../Source/JavaScriptCore/heap/Heap.h:32,
                 from ../../Source/JavaScriptCore/heap/DeferGC.h:29,
                 from ../../Source/JavaScriptCore/runtime/ConcurrentJSLock.h:28,
                 from ../../Source/JavaScriptCore/bytecode/ArrayProfile.h:28,
                 from ../../Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:29:
../../Source/JavaScriptCore/runtime/JSCConfig.h:108:69: error: static assertion failed
  108 | static_assert(WTF::offsetOfWTFConfigExtension + sizeof(JSC::Config) <= WTF::ConfigSizeToProtect);
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

[Michael Catanzaro]

OK, I don't know what to do here. The values are:

WTF::offsetOfWTFConfigExtension = 1312
sizeof(JSC::Config) = 15544
WTF::ConfigSizeToProtect = 16384

And 1312 + 15544 = 16856, too big. Before this patch, sizeof(JSC::Config) was 13856, way smaller. I'm confused because numberOfGates is 1, so it appears as if we only added one pointer to JSC::Config, not nearly 2 KB. Any ideas what happened here?

The only solution I was able to find is to increase ConfigSizeToProtect, which I understand is unlikely to be accepted. I'm pretty surprised to see the config size exceed 16 KB, but seems that has happened. Anyone have any great ideas? (I also tried disabling ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD) in PlatformEnable.h, which could also be a possible solution, but that needs some additional work to make it build.)

diff --git a/Source/WTF/wtf/WTFConfig.h b/Source/WTF/wtf/WTFConfig.h
index 37e6ba961262..db65b1dd3e0a 100644
--- a/Source/WTF/wtf/WTFConfig.h
+++ b/Source/WTF/wtf/WTFConfig.h
@@ -54,7 +54,7 @@ extern "C" WTF_EXPORT_PRIVATE Slot g_config[];
 namespace WTF {
 
 constexpr size_t ConfigAlignment = CeilingOnPageSize;
-constexpr size_t ConfigSizeToProtect = std::max(CeilingOnPageSize, 16 * KB);
+constexpr size_t ConfigSizeToProtect = std::max(CeilingOnPageSize, 32 * KB);
 
 struct Config {
     WTF_EXPORT_PRIVATE static void permanentlyFreeze();

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #24)
> OK, I don't know what to do here. The values are:
> 
> WTF::offsetOfWTFConfigExtension = 1312
> sizeof(JSC::Config) = 15544
> WTF::ConfigSizeToProtect = 16384
> 
> And 1312 + 15544 = 16856, too big. Before this patch, sizeof(JSC::Config)
> was 13856, way smaller. I'm confused because numberOfGates is 1, so it
> appears as if we only added one pointer to JSC::Config, not nearly 2 KB. Any
> ideas what happened here?
> 
> The only solution I was able to find is to increase ConfigSizeToProtect,
> which I understand is unlikely to be accepted. I'm pretty surprised to see
> the config size exceed 16 KB, but seems that has happened. Anyone have any
> great ideas? (I also tried disabling
> ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD) in PlatformEnable.h, which could
> also be a possible solution, but that needs some additional work to make it
> build.)
> 
> diff --git a/Source/WTF/wtf/WTFConfig.h b/Source/WTF/wtf/WTFConfig.h
> index 37e6ba961262..db65b1dd3e0a 100644
> --- a/Source/WTF/wtf/WTFConfig.h
> +++ b/Source/WTF/wtf/WTFConfig.h
> @@ -54,7 +54,7 @@ extern "C" WTF_EXPORT_PRIVATE Slot g_config[];
>  namespace WTF {
>  
>  constexpr size_t ConfigAlignment = CeilingOnPageSize;
> -constexpr size_t ConfigSizeToProtect = std::max(CeilingOnPageSize, 16 * KB);
> +constexpr size_t ConfigSizeToProtect = std::max(CeilingOnPageSize, 32 * KB);
>  
>  struct Config {
>      WTF_EXPORT_PRIVATE static void permanentlyFreeze();


Which build bot is showing failure? I see CLoop bot, and it is working.
This looks very strange since ENABLE(JIT_OPERATION_VALIDATION) is disabled for non JIT. So numberOfGates is 1.
https://build.webkit.org/builders/Apple-Catalina-LLINT-CLoop-BuildAndTest

[Yusuke Suzuki]

Committed r269378: <https://trac.webkit.org/changeset/269378>

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #25)
> Which build bot is showing failure? I see CLoop bot, and it is working.

It's failing on Red Hat's internal x86_64 cloop and aarch64 CI. I can also reproduce this locally on x86_64 (that's how I got specific numbers, and also confirmed numberOfGates == 1). I'm using -DPORT=JSCOnly to make the build fail faster, and -DENABLE_JIT=OFF -DENABLE_C_LOOP=ON -DENABLE_SAMPLING_PROFILER=OFF -DUSE_SYSTEM_MALLOC=ON to emulate aarch64.

> This looks very strange since ENABLE(JIT_OPERATION_VALIDATION) is disabled
> for non JIT. So numberOfGates is 1.
> https://build.webkit.org/builders/Apple-Catalina-LLINT-CLoop-BuildAndTest

Yes, numberOfGates is definitely 1. That is why I'm so confused. I don't understand how adding one pointer can affect the size of the struct so much. Is there anything else in this commit that might have possibly affected the config size?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #27)
> Yes, numberOfGates is definitely 1. That is why I'm so confused. I don't
> understand how adding one pointer can affect the size of the struct so much.
> Is there anything else in this commit that might have possibly affected the
> config size?

Ah, well obvious answer wins here... the new gateMap member is not to blame. I commented it out and changed #if ENABLE(JIT_CAGE) in LLintData.cpp to #if 0 in order to remove the code that uses it. The size of JSCConfig dropped from 15544 bytes to 15536, exactly one pointer, as expected. So it seems something else in this commit asides from that must have somehow affected JSCConfig....

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #28)
> (In reply to Michael Catanzaro from comment #27)
> > Yes, numberOfGates is definitely 1. That is why I'm so confused. I don't
> > understand how adding one pointer can affect the size of the struct so much.
> > Is there anything else in this commit that might have possibly affected the
> > config size?
> 
> Ah, well obvious answer wins here... the new gateMap member is not to blame.
> I commented it out and changed #if ENABLE(JIT_CAGE) in LLintData.cpp to #if
> 0 in order to remove the code that uses it. The size of JSCConfig dropped
> from 15544 bytes to 15536, exactly one pointer, as expected. So it seems
> something else in this commit asides from that must have somehow affected
> JSCConfig....

I built CLoop build with JSCOnly on macOS successfully. So I think this is Linux platform specific problem. Can you take a look into what is happening?

[Michael Catanzaro]

Yes, I've been narrowing this down further. It seems the size of the llint member has increased from 11440 bytes to 13128 bytes. Each of the three Opcode arrays has increased from 3792 bytes to 4352 bytes, and that difference is enough to cause JSCConfig to exceed 16 KB.

Originally, numOpcodeIDs was 272 and numWasmOpcodeIDs was 202. But with this patch, those have increased to 305 and 239, respectively. So it's just too many new opcodes to fit.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #30)
> Yes, I've been narrowing this down further. It seems the size of the llint
> member has increased from 11440 bytes to 13128 bytes. Each of the three
> Opcode arrays has increased from 3792 bytes to 4352 bytes, and that
> difference is enough to cause JSCConfig to exceed 16 KB.
> 
> Originally, numOpcodeIDs was 272 and numWasmOpcodeIDs was 202. But with this
> patch, those have increased to 305 and 239, respectively. So it's just too
> many new opcodes to fit.

But macOS does not exceed it. Is there any Linux specific hack?

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #31)
> (In reply to Michael Catanzaro from comment #30)
> > Yes, I've been narrowing this down further. It seems the size of the llint
> > member has increased from 11440 bytes to 13128 bytes. Each of the three
> > Opcode arrays has increased from 3792 bytes to 4352 bytes, and that
> > difference is enough to cause JSCConfig to exceed 16 KB.
> > 
> > Originally, numOpcodeIDs was 272 and numWasmOpcodeIDs was 202. But with this
> > patch, those have increased to 305 and 239, respectively. So it's just too
> > many new opcodes to fit.
> 
> But macOS does not exceed it. Is there any Linux specific hack?

For now, I'll increase Config size for OS(LINUX).

[Michael Catanzaro]

CC: Guij and Xan due to Linux-specific change proposal

Is there any Linux-specific hack? I don't know. Maybe you could check the size of some of these struct members on macOS and see how they compare to Linux? My guess is you're probably pretty close to exceeding 16 KB too?

Before (x86_64 cloop):

WTF::offsetOfWTFConfigExtension = 1312
Maximum allowable sizeof(JSC::Config) = 16384 - 1312 = 15072
sizeof(JSC::Config) = 13856
sizeof(llint) = 11440
sizeof(opcodeMap), sizeof(opcodeMapWide16), sizeof(opcodeMapWide32) = 3792
numOpcodeIDs = 272
numWasmOpcodeIDs = 202

After (x86_64 cloop):

WTF::offsetOfWTFConfigExtension = 1312
Maximum allowable sizeof(JSC::Config) = 16384 - 1312 = 15072
sizeof(JSC::Config) = 15544
sizeof(llint) = 13128
sizeof(opcodeMap), sizeof(opcodeMapWide16), sizeof(opcodeMapWide32) = 4352
numOpcodeIDs = 305
numWasmOpcodeIDs = 239

After (x86_64, normal build with JIT enabled):

WTF::offsetOfWTFConfigExtension = 1360
Maximum allowable sizeof(JSC::Config) = 16384 - 1360 = 15024
Actual sizeof(JSC::Config) = 14344
sizeof(llint) = 11928
sizeof(opcodeMap) = 3952
numOpcodeIDs = 255
numWasmOpcodeIDs = 239

I'm using this hack I stole from Stack Overflow to print the sizes as template parameters in compiler warnings, printing one at a time because GCC sometimes reports only the first warning:

diff --git a/Source/JavaScriptCore/runtime/JSCConfig.h b/Source/JavaScriptCore/runtime/JSCConfig.h
index 7f96ec52ed21..6d0024f4e201 100644
--- a/Source/JavaScriptCore/runtime/JSCConfig.h
+++ b/Source/JavaScriptCore/runtime/JSCConfig.h
@@ -105,11 +105,25 @@ struct Config {
 
 constexpr size_t alignmentOfJSCConfig = std::alignment_of<JSC::Config>::value;
 
+#define g_jscConfig (*bitwise_cast<JSC::Config*>(&g_wtfConfig.spaceForExtensions))
+
+template <auto val>
+constexpr void static_print() {
+    #if !defined(__GNUC__) || defined(__clang__)
+        int static_print_is_implemented_only_for_gcc = 0;
+    #else
+        int unused = 0;
+    #endif
+};
+
+static void whatever()
+{
+    static_print<sizeof(g_jscConfig.llint)>();
+}
+
 static_assert(WTF::offsetOfWTFConfigExtension + sizeof(JSC::Config) <= WTF::ConfigSizeToProtect);
 static_assert(roundUpToMultipleOf<alignmentOfJSCConfig>(WTF::offsetOfWTFConfigExtension) == WTF::offsetOfWTFConfigExtension);
 
-#define g_jscConfig (*bitwise_cast<JSC::Config*>(&g_wtfConfig.spaceForExtensions))
-
 #else // not ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD)
 
 extern "C" JS_EXPORT_PRIVATE Config g_jscConfig;

[Yusuke Suzuki]

Committed r269403: <https://trac.webkit.org/changeset/269403>

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #33)
> CC: Guij and Xan due to Linux-specific change proposal
> 
> Is there any Linux-specific hack? I don't know. Maybe you could check the
> size of some of these struct members on macOS and see how they compare to
> Linux? My guess is you're probably pretty close to exceeding 16 KB too?
> 
> Before (x86_64 cloop):
> 
> WTF::offsetOfWTFConfigExtension = 1312
> Maximum allowable sizeof(JSC::Config) = 16384 - 1312 = 15072
> sizeof(JSC::Config) = 13856
> sizeof(llint) = 11440
> sizeof(opcodeMap), sizeof(opcodeMapWide16), sizeof(opcodeMapWide32) = 3792
> numOpcodeIDs = 272
> numWasmOpcodeIDs = 202
> 
> After (x86_64 cloop):
> 
> WTF::offsetOfWTFConfigExtension = 1312
> Maximum allowable sizeof(JSC::Config) = 16384 - 1312 = 15072
> sizeof(JSC::Config) = 15544
> sizeof(llint) = 13128
> sizeof(opcodeMap), sizeof(opcodeMapWide16), sizeof(opcodeMapWide32) = 4352
> numOpcodeIDs = 305
> numWasmOpcodeIDs = 239
> 
> After (x86_64, normal build with JIT enabled):
> 
> WTF::offsetOfWTFConfigExtension = 1360
> Maximum allowable sizeof(JSC::Config) = 16384 - 1360 = 15024
> Actual sizeof(JSC::Config) = 14344
> sizeof(llint) = 11928
> sizeof(opcodeMap) = 3952
> numOpcodeIDs = 255
> numWasmOpcodeIDs = 239
> 
> I'm using this hack I stole from Stack Overflow to print the sizes as
> template parameters in compiler warnings, printing one at a time because GCC
> sometimes reports only the first warning:
> 
> diff --git a/Source/JavaScriptCore/runtime/JSCConfig.h
> b/Source/JavaScriptCore/runtime/JSCConfig.h
> index 7f96ec52ed21..6d0024f4e201 100644
> --- a/Source/JavaScriptCore/runtime/JSCConfig.h
> +++ b/Source/JavaScriptCore/runtime/JSCConfig.h
> @@ -105,11 +105,25 @@ struct Config {
>  
>  constexpr size_t alignmentOfJSCConfig =
> std::alignment_of<JSC::Config>::value;
>  
> +#define g_jscConfig
> (*bitwise_cast<JSC::Config*>(&g_wtfConfig.spaceForExtensions))
> +
> +template <auto val>
> +constexpr void static_print() {
> +    #if !defined(__GNUC__) || defined(__clang__)
> +        int static_print_is_implemented_only_for_gcc = 0;
> +    #else
> +        int unused = 0;
> +    #endif
> +};
> +
> +static void whatever()
> +{
> +    static_print<sizeof(g_jscConfig.llint)>();
> +}
> +
>  static_assert(WTF::offsetOfWTFConfigExtension + sizeof(JSC::Config) <=
> WTF::ConfigSizeToProtect);
>  static_assert(roundUpToMultipleOf<alignmentOfJSCConfig>(WTF::
> offsetOfWTFConfigExtension) == WTF::offsetOfWTFConfigExtension);
>  
> -#define g_jscConfig
> (*bitwise_cast<JSC::Config*>(&g_wtfConfig.spaceForExtensions))
> -
>  #else // not ENABLE(UNIFIED_AND_FREEZABLE_CONFIG_RECORD)
>  
>  extern "C" JS_EXPORT_PRIVATE Config g_jscConfig;

I think this is because Linux's sigaction is significantly larger than Darwin's one, and causing very large offsetOfWTFConfigExtension.
For now, I increased the Config size only for non Darwin builds. And we will look into https://bugs.webkit.org/show_bug.cgi?id=218592.
But https://bugs.webkit.org/show_bug.cgi?id=218592 requires clang / dyld's __DATA_CONST's truly read-only-ness support, so we need to wait for the completeness of __DATA_CONST work in clang / dyld side.

[Michael Catanzaro]

OK, thanks Yusuke!

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #36)
> OK, thanks Yusuke!

Thank you for looking into it!

[Yusuke Suzuki]

Committed r269677: <https://trac.webkit.org/changeset/269677>