Bug 97162

Summary: OSR exit sometimes neglects to create the arguments object
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, fpizlo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch fpizlo: review+

Description Geoffrey Garen 2012-09-19 19:08:59 PDT
OSR exit sometimes neglects to create the arguments object
Comment 1 Geoffrey Garen 2012-09-19 19:43:02 PDT
Created attachment 164827 [details]
Patch
Comment 2 Gyuyoung Kim 2012-09-19 19:47:43 PDT
Comment on attachment 164827 [details]
Patch

Attachment 164827 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13950032
Comment 3 Geoffrey Garen 2012-09-19 19:53:46 PDT
Created attachment 164828 [details]
Patch
Comment 4 Filip Pizlo 2012-09-19 20:54:29 PDT
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.
Comment 5 Filip Pizlo 2012-09-19 20:56:12 PDT
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.
Comment 6 Geoffrey Garen 2012-09-19 21:05:06 PDT
> 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.
Comment 7 Geoffrey Garen 2012-09-19 21:44:10 PDT
Committed r129089: <http://trac.webkit.org/changeset/129089>