Bug 218143 - [JSC] Add JITCage support
Summary: [JSC] Add JITCage support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 218583
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-23 17:51 PDT by Yusuke Suzuki
Modified: 2020-11-11 01:06 PST (History)
18 users (show)

See Also:


Attachments
Patch (164.02 KB, patch)
2020-10-23 17:53 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (164.51 KB, patch)
2020-10-23 18:52 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (164.76 KB, patch)
2020-10-23 19:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (165.60 KB, patch)
2020-10-23 23:24 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (168.45 KB, patch)
2020-10-27 17:25 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (168.45 KB, patch)
2020-10-27 19:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (168.60 KB, patch)
2020-10-28 15:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (168.61 KB, patch)
2020-10-28 17:43 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (168.58 KB, patch)
2020-10-28 18:03 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (168.64 KB, patch)
2020-10-29 00:11 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (165.26 KB, patch)
2020-10-29 00:35 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (167.74 KB, patch)
2020-11-03 15:29 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-10-23 17:51:51 PDT
[JSC] Add JITCage support
Comment 1 Yusuke Suzuki 2020-10-23 17:53:26 PDT
Created attachment 412218 [details]
Patch
Comment 2 Yusuke Suzuki 2020-10-23 18:52:23 PDT
Created attachment 412222 [details]
Patch
Comment 3 Yusuke Suzuki 2020-10-23 19:06:05 PDT
Created attachment 412223 [details]
Patch
Comment 4 Yusuke Suzuki 2020-10-23 23:24:36 PDT
Created attachment 412236 [details]
Patch
Comment 5 Yusuke Suzuki 2020-10-27 17:25:42 PDT
Created attachment 412483 [details]
Patch
Comment 6 Yusuke Suzuki 2020-10-27 19:50:07 PDT
Created attachment 412500 [details]
Patch
Comment 7 Caio Lima 2020-10-28 10:43:02 PDT
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.
Comment 8 Yusuke Suzuki 2020-10-28 15:05:39 PDT
Make sense!
Comment 9 Yusuke Suzuki 2020-10-28 15:06:20 PDT
Created attachment 412574 [details]
Patch
Comment 10 Yusuke Suzuki 2020-10-28 17:43:17 PDT
Created attachment 412594 [details]
Patch
Comment 11 Yusuke Suzuki 2020-10-28 18:03:45 PDT
Created attachment 412597 [details]
Patch
Comment 12 Yusuke Suzuki 2020-10-29 00:11:58 PDT
Created attachment 412620 [details]
Patch
Comment 13 Yusuke Suzuki 2020-10-29 00:35:13 PDT
Created attachment 412621 [details]
Patch
Comment 14 Radar WebKit Bug Importer 2020-10-30 17:52:16 PDT
<rdar://problem/70904668>
Comment 15 Caio Lima 2020-11-02 10:54:28 PST
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.
Comment 16 Saam Barati 2020-11-02 12:31:18 PST
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?
Comment 17 Saam Barati 2020-11-02 12:32:06 PST
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.
Comment 18 Mark Lam 2020-11-02 15:49:15 PST
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.
Comment 19 Yusuke Suzuki 2020-11-03 15:25:29 PST
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.
Comment 20 Yusuke Suzuki 2020-11-03 15:29:07 PST
Created attachment 413114 [details]
Patch for landing
Comment 21 Yusuke Suzuki 2020-11-03 18:32:02 PST
Committed r269349: <https://trac.webkit.org/changeset/269349>
Comment 22 Yusuke Suzuki 2020-11-03 20:55:49 PST
Committed r269353: <https://trac.webkit.org/changeset/269353>
Comment 23 Michael Catanzaro 2020-11-04 08:53:07 PST
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);
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 24 Michael Catanzaro 2020-11-04 09:54:44 PST
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();
Comment 25 Yusuke Suzuki 2020-11-04 12:19:57 PST
(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
Comment 26 Yusuke Suzuki 2020-11-04 13:05:54 PST
Committed r269378: <https://trac.webkit.org/changeset/269378>
Comment 27 Michael Catanzaro 2020-11-04 14:48:57 PST
(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?
Comment 28 Michael Catanzaro 2020-11-04 14:56:50 PST
(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....
Comment 29 Yusuke Suzuki 2020-11-04 15:16:48 PST
(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?
Comment 30 Michael Catanzaro 2020-11-04 15:39:30 PST
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.
Comment 31 Yusuke Suzuki 2020-11-04 15:52:13 PST
(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?
Comment 32 Yusuke Suzuki 2020-11-04 16:04:44 PST
(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).
Comment 33 Michael Catanzaro 2020-11-04 16:26:56 PST
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;
Comment 34 Yusuke Suzuki 2020-11-04 16:36:34 PST
Committed r269403: <https://trac.webkit.org/changeset/269403>
Comment 35 Yusuke Suzuki 2020-11-04 16:40:48 PST
(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.
Comment 36 Michael Catanzaro 2020-11-04 16:44:13 PST
OK, thanks Yusuke!
Comment 37 Yusuke Suzuki 2020-11-04 17:04:05 PST
(In reply to Michael Catanzaro from comment #36)
> OK, thanks Yusuke!

Thank you for looking into it!
Comment 38 Yusuke Suzuki 2020-11-11 01:06:37 PST
Committed r269677: <https://trac.webkit.org/changeset/269677>