[webkit-reviews] review denied: [Bug 181599] PoisonedWriteBarrier : [Attachment 331219] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 181599: PoisonedWriteBarrier
https://bugs.webkit.org/show_bug.cgi?id=181599

Attachment 331219: patch

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




--- Comment #7 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 331219
  --> https://bugs.webkit.org/attachment.cgi?id=331219
patch

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

I think you have a bug.  Please fix.

> Source/JavaScriptCore/runtime/WriteBarrier.h:113
> +	   validateCell<T>(Traits::unwrap(cell));
> +	   return Traits::unwrap(cell);

I suggest pre-caching the unwrapped pointer:
    T* resultCell = Traits::unwrap(cell);
    validateCell<T>(resultCell);
    return resultCell;

> Source/JavaScriptCore/runtime/WriteBarrier.h:121
> +	   validateCell(Traits::unwrap(cell));
> +	   return Traits::unwrap(cell);

Ditto.	precache?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:482
> +   
jit.move(CCallHelpers::TrustedImm64(JSWebAssemblyInstance::PoisonedBarrier<WebA
ssemblyToJSCallee>::poison), importJSCellGPRReg);

It is wrong to assume that importJSCellGPRReg (i.e. GPRInfo::regT0) !==
GPRInfo::argumentGPR0.	In fact, on ARM64 (and most other architectures), they
are the same.  You're just lucky that they aren't the same on x86_64 here. 
Please fix.