IonMonkey: Differential Testing: Getting different TypeErrors ((void 0) vs. 0) without/with ion.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks: 2 bugs, {regression, testcase})

Other Branch
x86_64
Linux
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ion:p1:fx18])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The following testcase shows different behavior with options --ion -n -m --ion-eager vs. --no-ion on ionmonkey revision c4c50dc6317c:


function printStatus (msg) {
  msg = msg.toString();
}
Function.prototype.toString ^= function () f(apply);
printStatus ("foo");
printStatus(let (printStatus) function() {});


$ debug64/js --ion -n -m --ion-eager test.js
test.js:2: TypeError: 0 is not a function

$ debug64/js --no-ion test.js
test.js:2: TypeError: (void 0) is not a function
On the given cset, I get: test.js:2: TypeError: msg.toString is not a function

Since the same error type is actually throwing in both cases, this is most likely a DecompileValueGenerator difference so we can consider this low priority.
(Assignee)

Comment 2

5 years ago
This error appear on tbpl (Column J with Linux opt/pgo) and is critical for landing.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
It sounds like we'll have to figure out how to preserve this error message, or make the tests more lenient.
Whiteboard: [js:p1:fx18]
Whiteboard: [js:p1:fx18] → [ion:p1:fx18]
(Assignee)

Comment 4

5 years ago
Created attachment 647330 [details] [diff] [review]
Fix DecompileValueGenerator.

- Make sure ReconstructPCStack can skip JSOP_THROWING by using JSOP_GOTO if possible.
- Add slow IonMonkey slot iteration to report nice error messages in case of failures. (when possible)
Attachment #647330 - Flags: review?(dvander)
Comment on attachment 647330 [details] [diff] [review]
Fix DecompileValueGenerator.

Review of attachment 647330 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea re: abstracting the slot iteration.

::: js/src/jsopcode.cpp
@@ +5792,5 @@
> +             *
> +             * In case of IonMonkey, we are using the snapshot which has
> +             * captured a previous state, so it is likely that we may not be
> +             * able to recover an expression of an error thrown from an
> +             * IonMonkey frame.

You might be able to remove this comment addition? We'll have guarded before any error that could cause js_DecompileValueGenerator to run, so the frame will be in the interpreter. For other cases, they should be effectful, and will thus have a recent snapshot at the safepoint.

::: js/src/vm/Stack.h
@@ +1822,5 @@
>      Value       thisv() const;
>  
> +    // These are only valid for the last frame.
> +    size_t      numDynSlots() const;
> +    Value       dynSlotValue(size_t index) const;

A few naming nits:
 s/last/top/
 s/numDynSlots/numFrameSlots/
 s/dynSlotValue/frameSlotValue/

Maybe add a comment that the values in slots may be optimized away in JITs?
Attachment #647330 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/ec897e9588f4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.