[webkit-reviews] review denied: [Bug 229543] [JSC] ASSERT failed in stress/for-in-tests.js (32bit) : [Attachment 438037] v5

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied  review:
Bug 229543: [JSC] ASSERT failed in stress/for-in-tests.js (32bit)
https://bugs.webkit.org/show_bug.cgi?id=229543

Attachment 438037: v5

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




--- Comment #14 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 438037
  --> https://bugs.webkit.org/attachment.cgi?id=438037
v5

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

Ah, I found a bug.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4486
> +    SpeculateCellOperand baseOperand(this, m_graph.varArgChild(node, 0),
ManualOperandSpeculation);

If you want to use SpeculateCellOperand, then you need to use CellUse /
KnownCellUse for base edge. But we do not have that edge, and no speculate().
You need to attach CellUse / KnownCellUse in fixup phase in 32bit. (And you
also need to check the other EnumeratorGetByVal use to ensure that edge UseKind
is handled.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4488
> +    JSValueOperand property(this, m_graph.varArgChild(node, 1),
ManualOperandSpeculation);

This ManualOperandSpeculation is wrong: if you use ManualOperandSpeculation,
then you need to call `speculate(...)`.

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:131
> +	       // If propertyName is not a cell then we are in index+named
mode, so do what RecoverNameAndGetVal does.
> +	       JSString* string = enumerator->propertyNameAtIndex(index);
> +	       auto propertyName = string->toIdentifier(globalObject);
> +	       RETURN_IF_EXCEPTION(scope, { });
> +	       RELEASE_AND_RETURN(scope, baseValue.get(globalObject,
propertyName));

Why do we need this new code? Can you add a test which stress this code path?