[webkit-reviews] review granted: [Bug 197110] Remove Gigacage from arm64 and use PAC for arm64e instead : [Attachment 369192] Patch

Mon May 6 19:27:30 PDT 2019

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 197110: Remove Gigacage from arm64 and use PAC for arm64e instead
https://bugs.webkit.org/show_bug.cgi?id=197110

Attachment 369192: Patch

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

--- Comment #42 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 369192
  --> https://bugs.webkit.org/attachment.cgi?id=369192
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::?