[webkit-reviews] review denied: [Bug 221260] [JSC] Enable WasmLLInt on ARMv7 : [Attachment 460061] v13 - ARM64E fix

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Geza Lore <glore at igalia.com>'s request
for review:
Bug 221260: [JSC] Enable WasmLLInt on ARMv7
https://bugs.webkit.org/show_bug.cgi?id=221260

Attachment 460061: v13 - ARM64E fix

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




--- Comment #29 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 460061
  --> https://bugs.webkit.org/attachment.cgi?id=460061
v13 - ARM64E fix

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

>> Source/JavaScriptCore/wasm/WasmInstance.h:113
>>	    }
> 
> This is the only diff from the original reverted patch. (Now we tag
m_cachedMemory with the same key as we use for untagging it)

OK, I've looked into the issue.  The root cause of this issue is because you
changed m_cachedBoundsCheckingSize to be set to memory()->size() previously,
bit did not tag m_cachedMemory with the same.  Your current fix changes
m_cachedMemory to be tagged with memory()->size() also.

However, looking through the code, I see that m_cachedBoundsCheckingSize is
only used for this untagging m_cachedMemory.  Why did you change it to
memory()->size() in the first place?  Can you not leave it as it was
originally? 

In this patch, you also renamed boundsCheckingSize() to mappedCapacity(), and
mappedCapacity() isn't used anywhere currently.  I can see how the name
boundsCheckingSize() can be misleading if we don't use it for bounds checking
in the non-signaling case.  However, I think it's appropriate to use
mappedCapacity() as the tag value here instead of memory()->size(). 
mappedCapacity() never changes, but memory()->size() can change.  Let's stick
with using mappedCapacity() because we want the tag to be "constant"-ish.  Can
you change this code back to the way it was before and use mappedCapacity() as
the tag value instead?