[webkit-reviews] review granted: [Bug 212001] OSR loop entry to iterator_next generic needs to CheckNotEmpty on m_next : [Attachment 399605] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 212001: OSR loop entry to iterator_next generic needs to CheckNotEmpty on
m_next
https://bugs.webkit.org/show_bug.cgi?id=212001

Attachment 399605: Patch

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




--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 399605
  --> https://bugs.webkit.org/attachment.cgi?id=399605
Patch

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

> Source/JavaScriptCore/ChangeLog:15
> +	   If we happen to OSR enter into iterator_next during a for-of loop
> +	   that has only profiled a generic iterator but is actually running
> +	   a fast iterator we will incorrectly exit forward to the next
> +	   getter checkpoint. This is because we don't check the next
> +	   function is non empty when we only emit a generic iterator_next
> +	   bytecode. The fix for this is to simply put a CheckNotEmpty at the
> +	   top of the generic case. 99.9% of the time this check will be
> +	   eliminated so it doesn't really cost anything.

the explanation you gave me in Slack is much clearer than this. I'd try to
explain it like that in here.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7000
> +		   // Sanity check that we don't get here with a fast path
during OSR entry.

I think this deserves a proper explanation. "Sanity check", to me, implies this
should never happen. But it can happen, so let's describe how, and just say the
below code can't handle the empty value.