Empty statement after return prevents asm compilation

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bbouvier)

Tracking

Trunk
mozilla24
x86_64
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Similar issue for empty statements above the "Parameter Type Annotations" section of a function.
(Assignee)

Comment 2

4 years ago
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 :)
Assignee: general → bbouvier
Status: NEW → ASSIGNED
Attachment #756839 - Flags: feedback?(luke)
(Assignee)

Comment 3

4 years ago
Created attachment 756840 [details] [diff] [review]
Part 2 - Fix for empty statements in parameter declarations

Independent of the first patch.
Attachment #756840 - Flags: review?(luke)
(Assignee)

Comment 4

4 years ago
Created attachment 756847 [details] [diff] [review]
Part 2 - Fix for empty statements in parameter declarations

Forgot to qref.
Attachment #756840 - Attachment is obsolete: true
Attachment #756840 - Flags: review?(luke)
Attachment #756847 - Flags: review?(luke)

Comment 5

4 years ago
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.
Attachment #756839 - Flags: feedback?(luke)

Updated

4 years ago
Attachment #756847 - Flags: review?(luke) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 759841 [details] [diff] [review]
Part 1 - same patch addressing comments

The function FunctionReturnLastStatementOrNull was used only here, so I also removed it.
Attachment #756839 - Attachment is obsolete: true
Attachment #759841 - Flags: review?(luke)

Comment 7

4 years ago
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) {
Attachment #759841 - Flags: review?(luke) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 759876 [details] [diff] [review]
Part 1 - same patch addressing comments and nits

Carrying r+ as nits are fixed.
Attachment #759841 - Attachment is obsolete: true
Attachment #759876 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65235fd0146
https://hg.mozilla.org/integration/mozilla-inbound/rev/cac9a6c4ab75
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a65235fd0146
https://hg.mozilla.org/mozilla-central/rev/cac9a6c4ab75
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.