Bug 97162 - OSR exit sometimes neglects to create the arguments object
Summary: OSR exit sometimes neglects to create the arguments object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-19 19:08 PDT by Geoffrey Garen
Modified: 2012-09-19 21:44 PDT (History)
2 users (show)

See Also:


Attachments
Patch (16.48 KB, patch)
2012-09-19 19:43 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (16.64 KB, patch)
2012-09-19 19:53 PDT, Geoffrey Garen
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>