....
Created attachment 405627 [details] patch
Comment on attachment 405627 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405627&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:135 > + TrustedImm32 shiftAmount { 64 - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) }; Will switch to using Keith's new constant once it lands. > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1152 > + skip = branchPtr(Equal, storage, TrustedImmPtr(JSArrayBufferView::nullVectorPtr())); this is the change. > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1188 > + done.append(branchPtr(Equal, storage, TrustedImmPtr(JSArrayBufferView::nullVectorPtr()))); this is the changed line.
Created attachment 405646 [details] patch
Comment on attachment 405646 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405646&action=review r=me with some comments. > Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:135 > + TrustedImm32 shiftAmount { 64 - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) }; Nit: Can you use numberOfPACBits? > Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:137 > + lshift64(shiftAmount, target); > + urshift64(shiftAmount, target); I think this can probably be a single and instruction for each of the constants we currently use.
Created attachment 405710 [details] patch for landing
Committed r265151: <https://trac.webkit.org/changeset/265151> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405710 [details].
<rdar://problem/66391434>
Re-opened since this is blocked by bug 215074
Created attachment 424271 [details] patch
Comment on attachment 424271 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=424271&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:149 > + auto scratch = InvalidGPRReg; > + if (validateAuth) { > + scratch = getCachedMemoryTempRegisterIDAndInvalidate(); > + move(target, scratch); > + } > + > m_assembler.autdb(target, lengthGPR); > + > + if (validateAuth) { > + ASSERT(target != ARM64Registers::sp); > + removeArrayPtrTag(scratch); > + auto isValidPtr = branch64(Equal, scratch, target); > + breakpoint(0xc473); > + isValidPtr.link(this); > + } Let's do this instead: auto scratch = validateAuth ? getCachedMemoryTempRegisterIDAndInvalidate() : InvalidGPRReg; untagArrayPtr(length, target, validateAuth, scratch); > Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:157 > + ASSERT(LogicalImmediate::create64(nonPACBitsMask).isValid()); > + and64(TrustedImmPtr(nonPACBitsMask), target); Isn't nonPACBitsMask a member of CagedPtr? Did this actually build? > Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1074 > +#if CPU(ARM64E) > + done.append(branchPtr(Equal, storage, TrustedImmPtr(JSArrayBufferView::nullVectorPtr()))); > +#endif Isn't this also needed for x86_64 because null != neutered?
Comment on attachment 424271 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=424271&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:149 >> + } > > Let's do this instead: > > auto scratch = validateAuth ? getCachedMemoryTempRegisterIDAndInvalidate() : InvalidGPRReg; > untagArrayPtr(length, target, validateAuth, scratch); sounds good >> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:157 >> + and64(TrustedImmPtr(nonPACBitsMask), target); > > Isn't nonPACBitsMask a member of CagedPtr? Did this actually build? It's also defined above in this file: static constexpr uintptr_t nonPACBitsMask = (1ull << (64 - numberOfPACBits)) - 1; >> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1074 >> +#endif > > Isn't this also needed for x86_64 because null != neutered? Detached array buffer is zero on x86: void JSArrayBufferView::detach() { auto locker = holdLock(cellLock()); RELEASE_ASSERT(hasArrayBuffer()); RELEASE_ASSERT(!isShared()); m_length = 0; m_vector.clear(); }
Comment on attachment 424271 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=424271&action=review r=me >>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:157 >>> + and64(TrustedImmPtr(nonPACBitsMask), target); >> >> Isn't nonPACBitsMask a member of CagedPtr? Did this actually build? > > It's also defined above in this file: > static constexpr uintptr_t nonPACBitsMask = (1ull << (64 - numberOfPACBits)) - 1; sorry, I had a bad grep. >>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1074 >>> +#endif >> >> Isn't this also needed for x86_64 because null != neutered? > > Detached array buffer is zero on x86: > void JSArrayBufferView::detach() > { > auto locker = holdLock(cellLock()); > RELEASE_ASSERT(hasArrayBuffer()); > RELEASE_ASSERT(!isShared()); > m_length = 0; > m_vector.clear(); > } I think it is a bug for x86_64 to use 0 to both mean null and neutered. But that's unrelated to this patch. So, moving on.
Created attachment 424284 [details] patch for landing
Committed r275079: <https://commits.webkit.org/r275079> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424284 [details].
*** Bug 210733 has been marked as a duplicate of this bug. ***