877965 - Empty statement after return prevents asm compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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]

Similar issue for empty statements above the "Parameter Type Annotations" section of a function.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Fix for empty statements in parameter declarations

Independent of the first patch.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Fix for empty statements in parameter declarations

Forgot to qref.

Comment 5

[Luke Wagner [:luke]]

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

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - same patch addressing comments

The function FunctionReturnLastStatementOrNull was used only here, so I also removed it.

Comment 7

[Luke Wagner [:luke]]

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

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1 - same patch addressing comments and nits

Carrying r+ as nits are fixed.

Comment 9

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a65235fd0146
https://hg.mozilla.org/integration/mozilla-inbound/rev/cac9a6c4ab75

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a65235fd0146
https://hg.mozilla.org/mozilla-central/rev/cac9a6c4ab75