[webkit-reviews] review granted: [Bug 190057] Verify the contents of AssemblerBuffer on arm64e : [Attachment 351035] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 190057: Verify the contents of AssemblerBuffer on arm64e
https://bugs.webkit.org/show_bug.cgi?id=190057

Attachment 351035: patch

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




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

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

r=me with remaining issues resolved.

I think you missed the ARMv7Assembler::fillNops().  Incidentally, the ARMv7
port does use the copy function in fillNops() and needs a fix.

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:973
>	   AssemblerType::fillNops(static_cast<char*>(buffer.data()) +
startCodeSize, memoryToFillWithNopsInBytes, isCopyingToExecutableMemory);

I think you need to update this to use the new fillNops() with memcpy. 
Strange, how did this build for x86_64 or any other non ARM64 build?  Oh, I get
it.  The copyFunction argument's type is templated, and on all those platforms,
the argument happens to be unused.  Hence, x86_64 doesn't care that we passed
it a boolean instead.  I think it's misleading to leave this code in this
state.	We should make it pass memcpy for the 3rd arg here.

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:174
> +		   )

Fix indentation.