Closed Bug 351219 Opened 16 years ago Closed 15 years ago

Decompiler outputs NaN or Infinity where it shouldn't

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha4

People

(Reporter: Seno.Aiko, Assigned: brendan)

References

Details

Attachments

(1 file)

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.
-0 needs special attention, too:

js> function f() { return -0 }
js> f
function f() {
    return 0;
}
*** Bug 352363 has been marked as a duplicate of this bug. ***
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})
See also bug 352360, "Negative zero decompiles as zero".
(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
(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
(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

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

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #259983 - Flags: review?(mrbkap)
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).
Attachment #259983 - Flags: review?(mrbkap) → review+
This should also fix bug 352360, if the stepping through the example I did recently is any guide.
I wish I knew the JSOP_GROUP fix's bug # -- Jesse, can you id it?

/be

Blocks: 352360
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha4
Fixed on trunk:

js/src/jsopcode.c 3.221

/be
Blocks: js1.7src
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #259983 - Flags: approval1.8.1.4?
Attachment #259983 - Flags: approval1.8.0.12?
Filed bug 375801 for not using "undefined".
> 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?
> I wish I knew the JSOP_GROUP fix's bug # -- Jesse, can you id it?

Bug 371692.
Blocks: 371692
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.
Attachment #259983 - Flags: approval1.8.1.4?
Attachment #259983 - Flags: approval1.8.1.4-
Attachment #259983 - Flags: approval1.8.0.12?
Attachment #259983 - Flags: approval1.8.0.12-
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
Attachment #259983 - Flags: review?(igor)
Attachment #259983 - Flags: approval1.8.1.4?
Attachment #259983 - Flags: approval1.8.1.4-
Attachment #259983 - Flags: approval1.8.0.12?
Attachment #259983 - Flags: approval1.8.0.12-
See also bug 375882, "Decompiler still outputs 'NaN' (etc) incorrectly for |case|"
The patch includes a fix to deal with trunk-only changes regarding loop bytecode  rearrangement and should not be applied to branches as-is?
(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
Attachment #259983 - Flags: review?(igor) → review+
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
Attachment #259983 - Flags: approval1.8.1.4?
Attachment #259983 - Flags: approval1.8.0.12?
(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
   ...
}

???
(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
/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-351219.js,v  <--  regress-351219.js
initial revision: 1.1
Flags: in-testsuite+
verified fixed linux, windows, mac* shell 20070406
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.