[webkit-reviews] review granted: [Bug 182006] Use precise index masking for FTL GetByArgumentByVal : [Attachment 332074] the patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 182006: Use precise index masking for FTL GetByArgumentByVal
https://bugs.webkit.org/show_bug.cgi?id=182006

Attachment 332074: the patch

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




--- Comment #4 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 332074
  --> https://bugs.webkit.org/attachment.cgi?id=332074
the patch

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

r=me with comments.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3987
> +	   LValue limit = m_out.sub(originalLimit, m_out.int32One);

Nit: I would make this 

LValue thisSize = m_out.int32One;
LValue limit = m_out.sub(originalLimit, thisSize);

I feel like I'll forget that the -1 is for this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4009
> +	   index = m_out.add(index, m_out.constInt32(1));

Nit: m_out.constInt32(1) => m_out.int32One.

Also, ditto on the thisSize.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4017
> +	   index = m_out.bitAnd(
> +	       index,
> +	       m_out.aShr(
> +		   m_out.sub(
> +		       index,
> +		       m_out.opaque(originalLimit)),
> +		   m_out.constInt32(31)));

Nit: I would do this math on 64-bit values. That avoids people passing very
large 32-bit indices and causes issues here.

E.g. arguments[UINT_MAX - epsilon] would produce a non-zero mask.