[webkit-reviews] review denied: [Bug 221260] [JSC] Enable WasmLLInt on ARMv7 : [Attachment 460061] v13 - ARM64E fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 7 18:05:27 PDT 2022
Yusuke Suzuki <ysuzuki at apple.com> has denied 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 #31 from Yusuke Suzuki <ysuzuki 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?
>
> I withdraw my comment. m_cachedBoundsCheckingSize is used in bounds checks
(I missed this previously). And this necessitates that it be set to
memory()->size().
>
> I've verified that updateCachedMemory() is called after the 2 places where
MemoryHandle::growToSize() is called. So, I think this is sound.
Wait, this is not correct.
For shared wasm memory, we are *intentionally* using mapped-capacity because of
growing feature from concurrently running wasm threads. Please read the commit
message in
https://github.com/WebKit/WebKit/commit/80581efa4a373a74da13227ba20e254b4efaa35
7
More information about the webkit-reviews
mailing list