Closed Bug 877965 Opened 7 years ago Closed 7 years ago

Empty statement after return prevents asm compilation


(Core :: JavaScript Engine, defect, minor)

Not set





(Reporter: jruderman, Assigned: bbouvier)



(2 files, 3 obsolete files)

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.
Similar issue for empty statements above the "Parameter Type Annotations" section of a function.
Attached patch Part 1 - Empty return fix + test (obsolete) — Splinter Review
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
Attachment #756839 - Flags: feedback?(luke)
Independent of the first patch.
Attachment #756840 - Flags: review?(luke)
Forgot to qref.
Attachment #756840 - Attachment is obsolete: true
Attachment #756840 - Flags: review?(luke)
Attachment #756847 - Flags: review?(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.
Attachment #756839 - Flags: feedback?(luke)
Attachment #756847 - Flags: review?(luke) → review+
The function FunctionReturnLastStatementOrNull was used only here, so I also removed it.
Attachment #756839 - Attachment is obsolete: true
Attachment #759841 - Flags: review?(luke)
Comment on attachment 759841 [details] [diff] [review]
Part 1 - same patch addressing comments

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


::: 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+
Carrying r+ as nits are fixed.
Attachment #759841 - Attachment is obsolete: true
Attachment #759876 - Flags: review+
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.