TI: Strict mode difference between TM and JM

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
RESOLVED WORKSFORME
7 years ago
5 years ago

People

(Reporter: azakai, Unassigned)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
I don't know which of TM and JM is correct, but JM recently changed its behavior in strict mode. The following code

try {
  throw 5;
} catch(e) {
  print(e + ',' + e.stack);
}

when run with -s (for strict mode) will show a warning (reference to undefined property e.stack) in JM but not in TM. JM didn't show the warning either a short time ago.

Comment 1

7 years ago
I can't reproduce a warning in JM revision 89d2095c7a87. I take it you mean with only -s, though I tried various combinations of -jmadws.
(Reporter)

Comment 2

7 years ago
Very odd. I just completely wiped js/, rebuilt at the revision you mentioned, and I get this:

~/Dev/jaegermonkey/js/src$ ./js -s
js> try { throw 5; } catch(e) { print(e + ',' + e.stack); }
typein:1: strict warning: reference to undefined property e.stack
5,undefined
js> 
~/Dev/jaegermonkey/js/src$ hg summary
parent: 68287:89d2095c7a87 
 [INFER] Only convert known ints when fixing doubles before branching, bug 652590.
branch: default

On tracemonkey I get

~/Dev/tracemonkey/js/src$ ./js -s
js> try { throw 5; } catch(e) { print(e + ',' + e.stack); }
5,undefined
js>

Comment 3

7 years ago
I can reproduce now. I think I was putting -s at the wrong side of the argument again.
Assignee: general → pbiggar

Comment 4

7 years ago
The warning is predicated on the opcode being a GETPROP which JM has, while TM has a GETLOCALPROP.

I recall bhackett removing some bytecodes which were optimizations in the interpreter, which was probably this. Looks to me like the warning is missing in TM, and should be easy to put back.

Comment 6

7 years ago
The productive way to deal with this is probably to get it into TM, so I'll have a look at that in the morning.

Comment 7

7 years ago
Created attachment 531015 [details] [diff] [review]
Port bug 647626 to TM

This is a straight port of bug 647626 to TM.
Attachment #531015 - Flags: review?(bhackett1024)
Comment on attachment 531015 [details] [diff] [review]
Port bug 647626 to TM

Yeah, this behavior is a bug in TM due to the compound opcodes (bug 647626, comment 7).
Attachment #531015 - Flags: review?(bhackett1024) → review+

Comment 9

7 years ago
Comment on attachment 531015 [details] [diff] [review]
Port bug 647626 to TM

Dão, fixing this bug creates a strict warning in browser.js. The patch fixes it. Can you confirm that it's the right fix? Is it OK to land this in tracemonkey, or should I split it out and land it in mozilla-central?
Attachment #531015 - Flags: review?(dao)
Comment on attachment 531015 [details] [diff] [review]
Port bug 647626 to TM

Landing this in tracemonkey should be fine.
Attachment #531015 - Flags: review?(dao) → review+

Comment 11

7 years ago
http://hg.mozilla.org/tracemonkey/rev/0c6254cb818d
Whiteboard: [fixed-in-tracemonkey]

Comment 12

7 years ago
Backout: http://hg.mozilla.org/tracemonkey/rev/0a2e66ce8631
Whiteboard: [fixed-in-tracemonkey]
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0c6254cb818d
http://hg.mozilla.org/mozilla-central/rev/0a2e66ce8631 (backout)
Note: not marking as fixed because last changeset is a backout.
(Reporter)

Comment 14

7 years ago
Any updates on this? Is it purposefully not being landed due to some problem (perhaps why it was backed out)?

Comment 15

7 years ago
I didn't land this because bhackett's type-inference merge subsumes it, and the merge is coming soon (I thought it was coming in early June, but I believe it is coming soon).

Brian, let me know if you think I should land this before you merge.
(Reporter)

Comment 16

7 years ago
Hmm, I think I am missing something here. So the correct behavior is on *JM*, and the incorrect in TM? (So a merge of JM to TM will fix it?) Which means both TM and JM were incorrect until this changed on JM (when I noticed it), at which point JM became correct?

Comment 17

7 years ago
Yes, that's correct.
(Reporter)

Comment 18

7 years ago
Got it, thanks. I don't see a reason to land this any earlier then.
js -s doesn't work anymore. However, when I run the comment #0 testcase with "use strict"; in front, I get the same results in Interp, JM, JM+TI, and v8.
Interp: 5,undefined
JM: 5,undefined
JM+TI: 5,undefined
d8: 5,undefined

I would call this WFM, but the earlier comments indicate that 5,undefined is not correct?

Updated

7 years ago
Assignee: paul.biggar → general
JM and TM were removed, getting same warning with various JIT flags if I add options("strict").
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.