455413 - TM: "Assertion failure: (m != JSVAL_INT) || isInt32(*vp)" with watch, pow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j 
js> this.watch('x', Math.pow); (function() { for(var j=0;j<4;++j){x=1;} })();

Assertion failure: (m != JSVAL_INT) || isInt32(*vp), at jstracer.cpp:1548

Comment 1

[Andreas Gal :gal]

Please make sure to put "TM:" in front of jit bugs.

Comment 2

[Andreas Gal :gal]

What exactly does watch do here?

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Looks like a data watchpoint: http://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/watch

Comment 4

[Andreas Gal :gal]

I will try to single-step through it.

Comment 5

[David Mandelin [:dmandelin]]

I stole this bug as a start on helping with TM bugs. Feel free to steal it back.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Patch

This patch should make SM refuse to start recording if any watchpoints are active. The check should be cheap; SunSpider with 50 trials came out 1.006x as fast.

Comment 7

[David Mandelin [:dmandelin]]

Attachment: Patch 2

Here's an improved patch that mrbkap suggested. It should more selectively abort only when the specific property being accessed has a watchpoint while still being a cheap check.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 341043 [details] [diff] [review]
Patch 2

r=me, thanks.

/be

Comment 9

[Andreas Gal :gal]

Comment on attachment 341032 [details] [diff] [review]
Patch

Looks good but I am afraid we need the same in js_ExecuteTree, otherwise we can execute trace code that ignores watchpoints.

Comment 10

[Andreas Gal :gal]

I think the generic method is good for js_ExecuteTree.

Comment 11

[Brendan Eich [:brendan]]

Adding a watchpoint for a property set that was recorded will change the shape. See jsscope.cpp, js_ChangeScopePropertyAttrs.

/be

Comment 12

[David Mandelin [:dmandelin]]

Patch 2 pushed as 71b214a5ecca. Does comment 11 mean that js_ExecuteTree is already taken care of by the flush-on-shape-change policy?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Patch 2 pushed as 71b214a5ecca. Does comment 11 mean that js_ExecuteTree is
> already taken care of by the flush-on-shape-change policy?

Write a test, prove me wrong :-).

/be

Comment 14

[Bob Clary [:bc] (inactive)]

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

http://hg.mozilla.org/mozilla-central/rev/b04c04268a94