Bug 197110 - Remove Gigacage from arm64 and use PAC for arm64e instead
Summary: Remove Gigacage from arm64 and use PAC for arm64e instead
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: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-19 11:12 PDT by Keith Miller
Modified: 2019-05-09 09:31 PDT (History)
12 users (show)

See Also:


Attachments
WIP (157.43 KB, patch)
2019-04-19 11:24 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
WIP (81.64 KB, patch)
2019-04-25 22:33 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
WIP (85.59 KB, patch)
2019-04-29 17:02 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Mostly done needs cleanup (131.19 KB, patch)
2019-05-03 17:41 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (128.86 KB, patch)
2019-05-06 10:00 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (135.93 KB, patch)
2019-05-06 10:11 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (135.95 KB, patch)
2019-05-06 11:07 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (136.39 KB, patch)
2019-05-06 11:13 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (127.67 KB, patch)
2019-05-06 11:24 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (128.17 KB, patch)
2019-05-06 11:28 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (128.21 KB, patch)
2019-05-06 11:39 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (809.18 KB, application/zip)
2019-05-06 12:49 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews101 for mac-highsierra (536.33 KB, application/zip)
2019-05-06 12:50 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.52 MB, application/zip)
2019-05-06 13:11 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews215 for win-future (13.57 MB, application/zip)
2019-05-06 13:40 PDT, EWS Watchlist
no flags Details
Patch (129.95 KB, patch)
2019-05-06 14:01 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.38 MB, application/zip)
2019-05-06 15:21 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.84 MB, application/zip)
2019-05-06 15:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.55 MB, application/zip)
2019-05-06 15:45 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.70 MB, application/zip)
2019-05-06 16:04 PDT, EWS Watchlist
no flags Details
Patch (128.15 KB, patch)
2019-05-06 16:16 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (3.51 MB, application/zip)
2019-05-06 19:05 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews213 for win-future (13.66 MB, application/zip)
2019-05-06 22:38 PDT, EWS Watchlist
no flags Details
Patch for landing (128.39 KB, patch)
2019-05-08 12:13 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2019-04-19 11:12:55 PDT
Remove Gigacage from arm64 and use PAC for arm64e instead
Comment 1 Keith Miller 2019-04-19 11:24:50 PDT
Created attachment 367808 [details]
WIP
Comment 2 Keith Miller 2019-04-25 22:33:41 PDT
Created attachment 368301 [details]
WIP
Comment 3 Keith Miller 2019-04-29 17:02:11 PDT
Created attachment 368517 [details]
WIP
Comment 4 Keith Miller 2019-05-03 17:41:13 PDT
Created attachment 369032 [details]
Mostly done needs cleanup
Comment 5 Keith Miller 2019-05-06 10:00:52 PDT
Created attachment 369130 [details]
Patch
Comment 6 EWS Watchlist 2019-05-06 10:02:13 PDT
Attachment 369130 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Keith Miller 2019-05-06 10:11:39 PDT
Created attachment 369131 [details]
Patch
Comment 8 EWS Watchlist 2019-05-06 10:14:48 PDT
Attachment 369131 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Keith Miller 2019-05-06 11:07:45 PDT
Created attachment 369141 [details]
Patch
Comment 10 EWS Watchlist 2019-05-06 11:10:38 PDT
Attachment 369141 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Keith Miller 2019-05-06 11:13:26 PDT
Created attachment 369144 [details]
Patch
Comment 12 EWS Watchlist 2019-05-06 11:16:50 PDT
Attachment 369144 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Keith Miller 2019-05-06 11:24:11 PDT
Created attachment 369145 [details]
Patch
Comment 14 EWS Watchlist 2019-05-06 11:27:04 PDT
Attachment 369145 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Keith Miller 2019-05-06 11:28:31 PDT
Created attachment 369146 [details]
Patch
Comment 16 EWS Watchlist 2019-05-06 11:30:35 PDT
Attachment 369146 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Keith Miller 2019-05-06 11:39:44 PDT
Created attachment 369150 [details]
Patch
Comment 18 EWS Watchlist 2019-05-06 11:41:26 PDT
Attachment 369150 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 EWS Watchlist 2019-05-06 12:49:47 PDT
Comment on attachment 369150 [details]
Patch

Attachment 369150 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12114098

Number of test failures exceeded the failure limit.
Comment 20 EWS Watchlist 2019-05-06 12:49:50 PDT
Created attachment 369158 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 21 EWS Watchlist 2019-05-06 12:50:12 PDT
Comment on attachment 369150 [details]
Patch

Attachment 369150 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12114119

Number of test failures exceeded the failure limit.
Comment 22 EWS Watchlist 2019-05-06 12:50:14 PDT
Created attachment 369159 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 23 EWS Watchlist 2019-05-06 13:11:46 PDT
Comment on attachment 369150 [details]
Patch

Attachment 369150 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12114185

Number of test failures exceeded the failure limit.
Comment 24 EWS Watchlist 2019-05-06 13:11:48 PDT
Created attachment 369162 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 25 EWS Watchlist 2019-05-06 13:40:32 PDT
Comment on attachment 369150 [details]
Patch

Attachment 369150 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12114406

Number of test failures exceeded the failure limit.
Comment 26 EWS Watchlist 2019-05-06 13:40:41 PDT
Created attachment 369164 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 27 Keith Miller 2019-05-06 14:01:11 PDT
Created attachment 369167 [details]
Patch
Comment 28 EWS Watchlist 2019-05-06 14:03:44 PDT
Attachment 369167 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 EWS Watchlist 2019-05-06 15:21:25 PDT
Comment on attachment 369167 [details]
Patch

Attachment 369167 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12115341

Number of test failures exceeded the failure limit.
Comment 30 EWS Watchlist 2019-05-06 15:21:27 PDT
Created attachment 369177 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 31 EWS Watchlist 2019-05-06 15:37:01 PDT
Comment on attachment 369167 [details]
Patch

Attachment 369167 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12115403

Number of test failures exceeded the failure limit.
Comment 32 EWS Watchlist 2019-05-06 15:37:03 PDT
Created attachment 369183 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 33 EWS Watchlist 2019-05-06 15:45:56 PDT
Comment on attachment 369167 [details]
Patch

Attachment 369167 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12115337

Number of test failures exceeded the failure limit.
Comment 34 EWS Watchlist 2019-05-06 15:45:58 PDT
Created attachment 369186 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 35 EWS Watchlist 2019-05-06 16:04:39 PDT
Comment on attachment 369167 [details]
Patch

Attachment 369167 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12115422

Number of test failures exceeded the failure limit.
Comment 36 EWS Watchlist 2019-05-06 16:04:41 PDT
Created attachment 369191 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4
Comment 37 Keith Miller 2019-05-06 16:16:19 PDT
Created attachment 369192 [details]
Patch
Comment 38 EWS Watchlist 2019-05-06 16:19:17 PDT
Attachment 369192 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:53:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:79:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 Saam Barati 2019-05-06 17:04:37 PDT
Comment on attachment 369192 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:105
> +    ALWAYS_INLINE void tagArrayPtr(RegisterID target, RegisterID length)
> +    {
> +        m_assembler.pacdb(target, length);
> +    }
> +
> +    ALWAYS_INLINE void untagArrayPtr(RegisterID target, RegisterID length)
> +    {
> +        m_assembler.autdb(target, length);
> +    }
> +
> +    ALWAYS_INLINE void untagArrayPtr(RegisterID target, Address length)
> +    {
> +        auto lengthGPR = getCachedDataTempRegisterIDAndInvalidate();
> +        load32(length, lengthGPR);
> +        m_assembler.autdb(target, lengthGPR);
> +    }
> +
> +    ALWAYS_INLINE void removeArrayPtrTag(RegisterID target)
> +    {
> +        m_assembler.xpacd(target);
> +    }

your destinations are on the wrong side.
Comment 40 EWS Watchlist 2019-05-06 19:05:22 PDT
Comment on attachment 369192 [details]
Patch

Attachment 369192 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12117975

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idb-binary-key-detached.htm
fast/canvas/webgl/arraybuffer-transfer-of-control.html
Comment 41 EWS Watchlist 2019-05-06 19:05:24 PDT
Created attachment 369217 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 42 Saam Barati 2019-05-06 19:27:30 PDT
Comment on attachment 369192 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        Change CagedBarrierPtr to work with PAC so constructors and accessors not expect to receive a length.

I'm confused in this sentence, I think you're missing a word somewhere.

> Source/JavaScriptCore/ChangeLog:15
> +        Add arm64e.rb backend as we missed that when originally open sourcing our code.

our code => our arm64e code

> Source/JavaScriptCore/ChangeLog:16
> +        Add a new optional t6 temporary, which is only used currently on arm64e for GetByVal on a TypedArray.

optional?

> Source/JavaScriptCore/ChangeLog:26
> +        Wasm:
> +        Use the TaggedArrayStoragePtr class for the memory base pointer. In theory we should be caging those pointers but I don't want to risk introducing a performance regression with the rest of this change. I've filed https://bugs.webkit.org/show_bug.cgi?id=197620 to do this later.

You should also say you're enabling fast memory. And perhaps also JS2 perf impact.

> Source/bmalloc/ChangeLog:9
> +        cage but returns a nullptr if the incoming pointer is already null.n

null.n => null.

> Source/JavaScriptCore/b3/B3ValueRep.h:79
> +        // that the use happens after any of the effects of the patchpoint.

effects => def. Perhaps instead just write "use interferes with the def"

> Source/JavaScriptCore/b3/B3ValueRep.h:81
> +        SomeLateRegister,

Can we have some patchpoint test where we assert this isn't the same as the input in a situation that forces the register allocator's hand to spill something perhaps?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2879
> +                GPRReg scratch = m_jit.scratchRegister();
> +                DisallowMacroScratchRegisterUsage disallowScratch(m_jit);

(This is kinda funky.)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2883
> +                m_jit.loadPtr(MacroAssembler::Address(base, JSArrayBufferView::offsetOfVector()), scratch);
> +                m_jit.removeArrayPtrTag(scratch);
> +                hasNullVector = m_jit.branchTestPtr(MacroAssembler::Zero, scratch);

Why not just do a `test` on the bits we care about? Probably can be encoded in a single insn too instead of having the strip.

Alternatively, null implies length of zero. So you could just tag nullptr with zero in this compiler thread and compare to that constant value. I kinda like my previous suggestion more though since I think it'll be fewer instructions.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14079
> +

style nit: remove this blank line

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14128
> +        if (kind == Gigacage::Primitive) {

Primitive always means typed arrays right now?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16558
> +#if !GIGACAGE_ENABLED && CPU(ARM64E)
> +        PatchpointValue* authenticate = m_out.patchpoint(pointerType());
> +        authenticate->appendSomeRegister(vector);
> +        authenticate->setGenerator([=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
> +            jit.move(params[1].gpr(), params[0].gpr());
> +            jit.removeArrayPtrTag(params[0].gpr());
> +        });
> +        vector = authenticate;
> +#endif

nit: Might be worth having a fully different code path instead of isZero64 here so B3 can nicely instruction select the test.

> Source/JavaScriptCore/jit/PolymorphicCallStubRoutine.cpp:-61
> -    if (Options::dumpDisassembly())
> -        dataLog("Clearing call link info for polymorphic call at ", m_callLinkInfo->callReturnLocation(), ", ", m_callLinkInfo->codeOrigin(), "\n");

probably worth reverting this?

> Source/JavaScriptCore/offlineasm/arm64e.rb:1
> +# Copyright (C) 2018 Apple Inc. All rights reserved.

2018-2019
Please also remove this file from the Internal repo (feel free to email me a bug link to that and I will review it).

> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:45
> +    m_destructor(m_data.getUnsafe());

why unsafe? Let's at least open a bug about fixing this.

> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68
> +    /* T& */ at(unsigned index, unsigned size) const { return get(size)[index]; }

did you mean to leave the comment here?

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:321
> +                    FINALIZE_CODE(linkBuffer, B3CompilationPtrTag, "WebAssembly BBQ function[%i] %s", functionIndex, signature.toString().ascii().data()),

nice

> Source/JavaScriptCore/wasm/WasmMemory.cpp:271
> +    this->memory();

what is this doing? Just debug assert?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:445
> +        memory();

Is this just for the debug assert?

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:227
> +            GPRReg scratch = jit.scratchRegister();
> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);

nit: let's just use Wasm::wasmCallingConventionAir().prologueScratch(0); instead of scratchRegister()

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:410
> +            GPRReg scratch = jit.scratchRegister();
> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> +

just use scratch2GPR here instead of this.

> Source/WTF/wtf/CagedUniquePtr.h:33
> +class CagedUniquePtr : public CagedPtr<kind, T, shouldTag> {

There doesn't appear to be a single place where shouldTag is true. Am I missing something?

> Source/WTF/wtf/PtrTag.h:155
> +template<typename T>
> +inline T* tagArrayPtr(nullptr_t ptr, size_t length)
> +{
> +    ASSERT(!length);
> +    return ptrauth_sign_unauthenticated(static_cast<T*>(ptr), ptrauth_key_process_dependent_data, length);
> +}
> +
> +
> +template<typename T>
> +inline T* tagArrayPtr(T* ptr, size_t length)
> +{
> +    return ptrauth_sign_unauthenticated(ptr, ptrauth_key_process_dependent_data, length);
> +}
> +
> +template<typename T>
> +inline T* untagArrayPtr(T* ptr, size_t length)
> +{
> +    return ptrauth_auth_data(ptr, ptrauth_key_process_dependent_data, length);
> +}
> +
> +template<typename T>
> +inline T* removeArrayPtrTag(T* ptr)
> +{
> +    return ptrauth_strip(ptr, ptrauth_key_process_dependent_data);
> +}
> +
> +template<typename T>
> +inline T* retagArrayPtr(T* ptr, size_t oldLength, size_t newLength)
> +{
> +    return ptrauth_auth_and_resign(ptr, ptrauth_key_process_dependent_data, oldLength, ptrauth_key_process_dependent_data, newLength);
> +}

can we add `using XYZ` for these functions so we don't have to prefix calls with WTF::?
Comment 43 EWS Watchlist 2019-05-06 22:38:40 PDT
Comment on attachment 369192 [details]
Patch

Attachment 369192 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12119340

New failing tests:
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html
Comment 44 EWS Watchlist 2019-05-06 22:38:43 PDT
Created attachment 369235 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 45 Keith Miller 2019-05-08 10:53:39 PDT
rdar://problem/48721296
Comment 46 Keith Miller 2019-05-08 12:12:06 PDT
Comment on attachment 369192 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        Change CagedBarrierPtr to work with PAC so constructors and accessors not expect to receive a length.
> 
> I'm confused in this sentence, I think you're missing a word somewhere.

it's a typo that's throwing you off: not => now.

>> Source/JavaScriptCore/ChangeLog:16
>> +        Add a new optional t6 temporary, which is only used currently on arm64e for GetByVal on a TypedArray.
> 
> optional?

It's optional in so far as it's not required to have it for every platform to work.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:105
>> +    }
> 
> your destinations are on the wrong side.

Fixed. I filed https://bugs.webkit.org/show_bug.cgi?id=197677 for the other functions.

>> Source/JavaScriptCore/b3/B3ValueRep.h:79
>> +        // that the use happens after any of the effects of the patchpoint.
> 
> effects => def. Perhaps instead just write "use interferes with the def"

done.

>> Source/JavaScriptCore/b3/B3ValueRep.h:81
>> +        SomeLateRegister,
> 
> Can we have some patchpoint test where we assert this isn't the same as the input in a situation that forces the register allocator's hand to spill something perhaps?

Sure.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2879
>> +                DisallowMacroScratchRegisterUsage disallowScratch(m_jit);
> 
> (This is kinda funky.)

Yeah, but it probably more efficient than using a scratch here? I suppose I could ask for some caller save register and clobber that but it seems like this is the most available scratch.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2883
>> +                hasNullVector = m_jit.branchTestPtr(MacroAssembler::Zero, scratch);
> 
> Why not just do a `test` on the bits we care about? Probably can be encoded in a single insn too instead of having the strip.
> 
> Alternatively, null implies length of zero. So you could just tag nullptr with zero in this compiler thread and compare to that constant value. I kinda like my previous suggestion more though since I think it'll be fewer instructions.

That *might* work because we don't support tagged pointers but I'd rather do it in a follow up.

Materializing the value is still at least one instruction. Additionally, IIRC, stripping the pointer is only 1 cycle.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14079
>> +
> 
> style nit: remove this blank line

Done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14128
>> +        if (kind == Gigacage::Primitive) {
> 
> Primitive always means typed arrays right now?

For this function yeah. We put other arguments data in the primitive cage but we access them through other helpers... :/

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16558
>> +#endif
> 
> nit: Might be worth having a fully different code path instead of isZero64 here so B3 can nicely instruction select the test.

Sure, I'll file a bug for this and add a FIXME.

>> Source/JavaScriptCore/jit/PolymorphicCallStubRoutine.cpp:-61
>> -        dataLog("Clearing call link info for polymorphic call at ", m_callLinkInfo->callReturnLocation(), ", ", m_callLinkInfo->codeOrigin(), "\n");
> 
> probably worth reverting this?

It's actually incorrect. Since we call this when we are in final path and codeOrigin() loads other cells we can crash.

>> Source/JavaScriptCore/offlineasm/arm64e.rb:1
>> +# Copyright (C) 2018 Apple Inc. All rights reserved.
> 
> 2018-2019
> Please also remove this file from the Internal repo (feel free to email me a bug link to that and I will review it).

Will do.

>> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:45
>> +    m_destructor(m_data.getUnsafe());
> 
> why unsafe? Let's at least open a bug about fixing this.

Done. https://bugs.webkit.org/show_bug.cgi?id=197698

>> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:68
>> +    /* T& */ at(unsigned index, unsigned size) const { return get(size)[index]; }
> 
> did you mean to leave the comment here?

Yeah, so the result type is more obviously visible. Otherwise you have to parse the entire enable_if... :(

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:271
>> +    this->memory();
> 
> what is this doing? Just debug assert?

Yeah, removed.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:445
>> +        memory();
> 
> Is this just for the debug assert?

Yeah, removed.

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:227
>> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> 
> nit: let's just use Wasm::wasmCallingConventionAir().prologueScratch(0); instead of scratchRegister()

Done.

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:410
>> +
> 
> just use scratch2GPR here instead of this.

Done.

>> Source/WTF/wtf/CagedUniquePtr.h:33
>> +class CagedUniquePtr : public CagedPtr<kind, T, shouldTag> {
> 
> There doesn't appear to be a single place where shouldTag is true. Am I missing something?

I exposed a global: constexpr bool tagCagedPtr = true, which I use as an "enum".

>> Source/WTF/wtf/PtrTag.h:155
>> +}
> 
> can we add `using XYZ` for these functions so we don't have to prefix calls with WTF::?

Done.
Comment 47 Keith Miller 2019-05-08 12:13:29 PDT
Created attachment 369405 [details]
Patch for landing
Comment 48 WebKit Commit Bot 2019-05-08 13:07:59 PDT
Comment on attachment 369405 [details]
Patch for landing

Clearing flags on attachment: 369405

Committed r245064: <https://trac.webkit.org/changeset/245064>
Comment 49 WebKit Commit Bot 2019-05-08 13:08:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Yusuke Suzuki 2019-05-08 22:41:07 PDT
Committed r245093: <https://trac.webkit.org/changeset/245093>