Bug 97162 – OSR exit sometimes neglects to create the arguments object

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

OSR exit sometimes neglects to create the arguments object

[Geoffrey Garen]

Created attachment 164827 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164827 [details]
Patch

Attachment 164827 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13950032

[Geoffrey Garen]

Created attachment 164828 [details]
Patch

[Filip Pizlo]

Comment on attachment 164828 [details]
Patch

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

Your call on the hash traits thingy.  I think they might be useful in other cases, too, so having them in the DFG, and particularly in the header of one phase of the DFG, seems awkward.  But I can't bring myself to care too much.

> Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.h:44
> +template<typename T>
> +struct NullableHashTraits : public HashTraits<T> {
> +    static const bool emptyValueIsZero = false;
> +    static T emptyValue() { return reinterpret_cast<T>(1); }
> +};
> +

It's not a big deal but it would be super fabulous if this was in HashTraits.h.

[Filip Pizlo]

Comment on attachment 164828 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp:434
>                             || unmodifiedArgumentsRegister(m_graph.argumentsRegisterFor(node.codeOrigin)) == variableAccessData->local()) {
> -                        // The child of this store should really be the empty value.
> -                        Node emptyJSValue(JSConstant, node.codeOrigin, OpInfo(codeBlock()->addOrFindConstant(JSValue())));
> -                        emptyJSValue.ref();
> -                        NodeIndex emptyJSValueIndex = m_graph.size();
> -                        m_graph.deref(node.child1());
> -                        node.children.child1() = Edge(emptyJSValueIndex);
> -                        m_graph.append(emptyJSValue);
> -                        insertionSet.append(indexInBlock, emptyJSValueIndex);
> -                        changed = true;
>                          break;
>                      }

Doesn't this create one of those situations where we would omit the { and }?  I think style checker is agnostic since the if itself spans two lines.  I would drop the { and }.  But it's not a big deal.

[Geoffrey Garen]

> It's not a big deal but it would be super fabulous if this was in HashTraits.h.

Will do.

> Doesn't this create one of those situations where we would omit the { and }?

You're right. Will fix.

[Geoffrey Garen]

Committed r129089: <http://trac.webkit.org/changeset/129089>