351219 - Decompiler outputs NaN or Infinity where it shouldn't

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Aiko]

The engines optimises some numerical expressions which is generally fine, but the result can be either (-)Infinity or NaN and the decompiler does not handle these cases well.

Two examples:

js> function f() { return 2e308 }
js> f
function f() {
    return Infinity;
}

That's just not the same.

js> function g() { var NaN = 1 % 0; return NaN }
js> dis(g)
main:
00000:  number NaN
00003:  setvar 0
00006:  pop
00007:  getvar 0
00010:  return
00011:  stop

Source notes:
  0:     3 [   3] decl     offset 0

That's fine, but...

js> g
function g() {
    var NaN = NaN;
    return NaN;
}

Ouch.

Suggested solution: decompile "number Infinity" as "1 / 0", "number -Infinity" as "1 / -0" and "number NaN" as "0 / 0", surrounded with parentheses if necessary.

Comment 1

[Aiko]

-0 needs special attention, too:

js> function f() { return -0 }
js> f
function f() {
    return 0;
}

Comment 2

[Aiko]

*** Bug 352363 has been marked as a duplicate of this bug. ***

Comment 3

[Jesse Ruderman]

Another bizarrely mutable property of the global object is "undefined".  What could be used in its place for decompilation?

js> uneval({a: undefined})
({a:undefined})

Comment 4

[Jesse Ruderman]

See also bug 352360, "Negative zero decompiles as zero".

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Another bizarrely mutable property of the global object is "undefined".  What
> could be used in its place for decompilation?

(void 0)

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Two examples:
> 
> js> function f() { return 2e308 }
> js> f
> function f() {
>     return Infinity;
> }
> 
> That's just not the same.

The decompilation is "the same", that is, when compiled it results in a function that returns the same value.

The only problem is the mutability of Infinity.

> Suggested solution: decompile "number Infinity" as "1 / 0", "number -Infinity"
> as "1 / -0" and "number NaN" as "0 / 0", surrounded with parentheses if
> necessary.

Patch next.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Another bizarrely mutable property of the global object is "undefined".  What
> could be used in its place for decompilation?
> 
> js> uneval({a: undefined})
> ({a:undefined})

Please file a separate bug on this. Thanks,

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix

Plus a fix to JSOP_GROUP's decompiler to cope with recent single-backward-branch loop codegen changes.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 259983 [details] [diff] [review]
fix

You might want to comment in the bug that the JSOP_GROUP change fixes (I don't have the number on hand right now).

Comment 10

[Jeff Walden [:Waldo]]

This should also fix bug 352360, if the stepping through the example I did recently is any guide.

Comment 11

[Brendan Eich [:brendan]]

I wish I knew the JSOP_GROUP fix's bug # -- Jesse, can you id it?

/be

Comment 12

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsopcode.c 3.221

/be

Comment 13

[Jesse Ruderman]

Filed bug 375801 for not using "undefined".

Comment 14

[Jesse Ruderman]

> I wish I knew the JSOP_GROUP fix's bug # -- Jesse, can you id it?

Is that a patch you attached to another bug but didn't check in?  Can you describe the bug it fixes?

Comment 15

[Jesse Ruderman]

> I wish I knew the JSOP_GROUP fix's bug # -- Jesse, can you id it?

Bug 371692.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 259983 [details] [diff] [review]
fix

Need more explanation of the harm this is causing on the branch, seems like a harmless edge case that can be left for the trunk.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 259983 [details] [diff] [review]
fix

It's a small correctness fix, safe by inspection. The added code calls SprintCString with string constants. The load from JSVAL_TO_DOUBLE(val) was in the old code already, in the call to JS_dtostr.

Asking igor for another review to help allay branch drivers' fears.

/be

Comment 18

[Jesse Ruderman]

See also bug 375882, "Decompiler still outputs 'NaN' (etc) incorrectly for |case|"

Comment 19

[Igor Bukanov]

The patch includes a fix to deal with trunk-only changes regarding loop bytecode  rearrangement and should not be applied to branches as-is?

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> The patch includes a fix to deal with trunk-only changes regarding loop
> bytecode  rearrangement and should not be applied to branches as-is?

Testing JSOP_IFEQ and JSOP_IFNE is future-proof and harmless on the branch. We need one of those ops to be tested on the branch (IFEQ, IIRC). I'm looking to fix this on the 1.8 branch at any rate -- arguably js1.7src should be close to that branch when we finalize js1.7. But if we have to have a mini-branch already, for other js1.7src changes not approved for 1.8.1.x, then perhaps this can wait.

It's a good fix for anyone fuzzing the branch, but Jesse isn't doing that atm, and he could avoid this bug (at some cost) if he were.

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 259983 [details] [diff] [review]
fix

Not so sure about this -- Waldo wants us to anticipate ES4 and make Infinity, NaN and undefined readonly/permanent. Never mind about the branches and js1.7src for now.

/be

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> (From update of attachment 259983 [details] [diff] [review])
> Not so sure about this -- Waldo wants us to anticipate ES4 and make Infinity,
> NaN and undefined readonly/permanent.

But how readonly/permanent top level properties would help to protect against variable names, function arguments or

var obj = {Infinity: 0};
with (obj) {
   // Infinity means 0 here
   ...
}

???

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 259983 [details] [diff] [review] [details])
> > Not so sure about this -- Waldo wants us to anticipate ES4 and make Infinity,
> > NaN and undefined readonly/permanent.
> 
> But how readonly/permanent top level properties would help to protect against
> variable names, function arguments or
> 
> var obj = {Infinity: 0};
> with (obj) {
>    // Infinity means 0 here
>    ...
> }
> 
> ???

It wouldn't. Only reserving identifiers, which ES4 won't do for these names, would do. I'll fix the case label bug and keep the js1.7src linkage, but wait on branch nomination.

/be

Comment 24

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-351219.js,v  <--  regress-351219.js
initial revision: 1.1

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed linux, windows, mac* shell 20070406