Last Comment Bug 877965 - Empty statement after return prevents asm compilation
: Empty statement after return prevents asm compilation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- minor (vote)
: mozilla24
Assigned To: (PTO until 29th August) Benjamin Bouvier [:bbouvier]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-30 22:03 PDT by Jesse Ruderman
Modified: 2013-06-10 02:15 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Empty return fix + test (3.48 KB, patch)
2013-05-31 16:08 PDT, (PTO until 29th August) Benjamin Bouvier [:bbouvier]
no flags Details | Diff | Splinter Review
Part 2 - Fix for empty statements in parameter declarations (2.36 KB, patch)
2013-05-31 16:09 PDT, (PTO until 29th August) Benjamin Bouvier [:bbouvier]
no flags Details | Diff | Splinter Review
Part 2 - Fix for empty statements in parameter declarations (2.42 KB, patch)
2013-05-31 16:27 PDT, (PTO until 29th August) Benjamin Bouvier [:bbouvier]
luke: review+
Details | Diff | Splinter Review
Part 1 - same patch addressing comments (3.45 KB, patch)
2013-06-07 11:20 PDT, (PTO until 29th August) Benjamin Bouvier [:bbouvier]
luke: review+
Details | Diff | Splinter Review
Part 1 - same patch addressing comments and nits (3.41 KB, patch)
2013-06-07 12:08 PDT, (PTO until 29th August) Benjamin Bouvier [:bbouvier]
bbouvier: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-05-30 22:03:38 PDT
function asm() {
    "use asm"
    function three() {
        return +3;
        ;
    }
    return { three: three };
}

asm.js type error: double is not a subtype of void

The spec has two seemingly contradictory requirements here:

> An asm.js function's return type is determined by the last statement in the 
> function body

> Empty statements (;) are always ignored, whether in the top level of a module or 
> inside an asm.js function body.
Comment 1 Jesse Ruderman 2013-05-30 22:30:07 PDT
Similar issue for empty statements above the "Parameter Type Annotations" section of a function.
Comment 2 (PTO until 29th August) Benjamin Bouvier [:bbouvier] 2013-05-31 16:08:03 PDT
Created attachment 756839 [details] [diff] [review]
Part 1 - Empty return fix + test

When checking for the return type, the use of last statement can result in a void return type instead of the real return type. If the list of parse nodes was double linked, the solution would be to loop backward until we find a return statement. This patch implements a loop from the beginning of the statement list of the function; so complexity becomes linear instead of constant, which is not really cool, but I don't see any other obvious solution.

Any precision about the spec or any other way to implement it would be appreciated :)
Comment 3 (PTO until 29th August) Benjamin Bouvier [:bbouvier] 2013-05-31 16:09:45 PDT
Created attachment 756840 [details] [diff] [review]
Part 2 - Fix for empty statements in parameter declarations

Independent of the first patch.
Comment 4 (PTO until 29th August) Benjamin Bouvier [:bbouvier] 2013-05-31 16:27:53 PDT
Created attachment 756847 [details] [diff] [review]
Part 2 - Fix for empty statements in parameter declarations

Forgot to qref.
Comment 5 Luke Wagner [:luke] 2013-06-06 18:35:30 PDT
Comment on attachment 756839 [details] [diff] [review]
Part 1 - Empty return fix + test

Review of attachment 756839 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this Benjamin!  As is, I think there is a bug that FunctionLastReturnStatement will return the first (but possibly not last) return statement.  Also, while I see that you are trying to make the slow linear search only occur on the fail path, I'd rather just keep it simple and *always* call FunctionLastReturnStatementOrNull (and can you append OrNull to the name)?  I doubt this will be a significant perf change.
Comment 6 (PTO until 29th August) Benjamin Bouvier [:bbouvier] 2013-06-07 11:20:46 PDT
Created attachment 759841 [details] [diff] [review]
Part 1 - same patch addressing comments

The function FunctionReturnLastStatementOrNull was used only here, so I also removed it.
Comment 7 Luke Wagner [:luke] 2013-06-07 11:48:45 PDT
Comment on attachment 759841 [details] [diff] [review]
Part 1 - same patch addressing comments

Review of attachment 759841 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/ion/AsmJS.cpp
@@ +278,5 @@
>  
>  static inline ParseNode *
> +FunctionLastReturnStatementOrNull(ParseNode *fn)
> +{
> +    JS_ASSERT(FunctionHasStatementList(fn));

FunctionStatementList() already does this assertion, so you can remove it here.

@@ +282,5 @@
> +    JS_ASSERT(FunctionHasStatementList(fn));
> +    ParseNode *listIter = ListHead(FunctionStatementList(fn));
> +    ParseNode *lastReturn = NULL;
> +    while (listIter)
> +    {

while (listIter) {
Comment 8 (PTO until 29th August) Benjamin Bouvier [:bbouvier] 2013-06-07 12:08:41 PDT
Created attachment 759876 [details] [diff] [review]
Part 1 - same patch addressing comments and nits

Carrying r+ as nits are fixed.

Note You need to log in before you can comment on or make changes to this bug.