[Webkit-unassigned] [Bug 221260] [JSC] Enable WasmLLInt on ARMv7

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=221260

Yusuke Suzuki <ysuzuki at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ysuzuki at apple.com
 Attachment #460061|review+                     |review-
              Flags|                            |

--- 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/80581efa4a373a74da13227ba20e254b4efaa357

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220608/a51a637a/attachment-0001.htm>