465443 - TM: "Assertion failed: p->isQuad()" with const-muddled for-each loop

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

const b = 16;
var out = [];
for each (b in [true, "", true, "", true, ""]) out.push(b >> 1);

Assertion failed: p->isQuad() (../nanojit/Nativei386.cpp:1601)

Comment 1

[Graydon Hoare :graydon]

Notes: 

Changing the const to a var makes this "work", but only in a pretty broken way: it keeps changing the type of b, so never manages to get a global object it recognizes the shape of. Just keeps throwing things out.

Keeping it 'const' won't change the ongoing writes to b, however, because the JIT doesn't currently enforce JSPROP_READONLY (see bug 465132). What it does instead is much worse: the interpreter *doesn't* write a GCTHING to the slot b, but the recorder believes it *did*. So the recorder updates its tracker with information that the slot contains the new GCTHING, even though the slot really still contains an int. As far as I can tell the tracker is doomed at this point. I'm not sure exactly which call dies first; the first f2i I see the recorder laying into the LIR buffer is already a bogus LIns (inside the generated call to js_DoubleToInt32 for the LHS of the right-shift expression, 'b') but the actual crash we see appears later in the code, when assembling a later call to js_DoubleToInt32 on, er ... looks like it's trying to promote the LIns of the return-stack pointer? Yuck. Still, given that we assemble backwards, the fact that this happens downstream of the initial corruption doesn't surprise me. I imagine the assembler would die at every subsequent instruction between where it dies and the point-of-corruption too. It's just stopping at the first bad thing it sees.

Solution? My guess is to bail off trace when we see JSPROP_READONLY, I'd say. Quick fix anyway. Deeper fix is to simulate what the interpreter does. I'll try to make a patch that does the former.

Comment 2

[Andreas Gal :gal]

Why don't we just don't emit write code and stay on trace? Isn't that was the interpreter essentially does? Not write?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Why don't we just don't emit write code and stay on trace? Isn't that was the
> interpreter essentially does? Not write?

The test fails at compile time if wrapped in a function. In global code we do not check at compile time that const b bound a read-only property. This difference is a bug, but const in global code is still up in the air in Ecma TC39. So we should fix this bug expeditiously: abort recording.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > Why don't we just don't emit write code and stay on trace? Isn't that was the
> > interpreter essentially does? Not write?
> 
> The test fails at compile time if wrapped in a function. In global code we do
> not check at compile time that const b bound a read-only property.

Sorry, I meant "check at compile time that the for-(each-)in loop variable is a const". We do emit JSOP_DEFCONST for const b, which calls js_CheckRedeclaration to error on a pre-existing var (read-write) binding named b.

/be

Comment 5

[Graydon Hoare :graydon]

Attachment: bail off trace if importing a global readonly slot

This also fixes bug 465132. I also added debug output to the rare case of import failure from global import table exhaustion. Comments welcome if you happen to know a faster way to go from slot number to sprop. I can't find one. Brendan?

A more complete version would track const bit in the tracker and simulate / omit writes. But considering const semantics aren't even clearly defined at the moment, I think such a version can wait.

Comment 6

[Andreas Gal :gal]

Comment on attachment 350499 [details] [diff] [review]
bail off trace if importing a global readonly slot

You could change the interface to accept a JSScopeProperty* instead of the slot number. That works for most callers excep SET/GETGVAR, which would have to do this lookup code (with a little helper).

Comment 7

[Graydon Hoare :graydon]

Uh oh. It appears to cause trace-tests.js to run out of memory in the mandelbrot test (which, erm, uses Math.E). Possibly I'll need to be fancier about tracking const in the imported slot and only bailing on writes-to-consts. Sigh.

Comment 8

[Andreas Gal :gal]

You don't need to track the const bit. You can look it up again as needed since you do have the slot number.

Comment 9

[Graydon Hoare :graydon]

(In reply to comment #8)
> You don't need to track the const bit. You can look it up again as needed since
> you do have the slot number.

At the cost of a linear search over the global property set per global write, during compilation. Yeah. If you aren't concerned with that cost, sure. Or in the tracker. I can do either.

There is also the matter of either simluating js_CheckRedeclaration or modifying, say, TraceRecorder::set to return boolean and modifying every caller of it to check and propagate its failure, possibly bailing off trace if it tries to write to a read-only. 

Any preferences?

Comment 10

[Andreas Gal :gal]

We already have an sprop pointer in most cases. What would we need to search? Or do you meat GET/SETGVAR?

Seems to me that we want to check for redeclaration before we enter into the ::set path since we use set for a bunch of things where redeclaration isn't an issue.

Comment 11

[Graydon Hoare :graydon]

Attachment: less pessimistic variant, still relatively minimal

This version threads a boolean "forWrite" parameter through to the lazy import check, such that it's only prevented from importing const globals about to be written to. Makes the change harmless for const-reading code. No longer makes mandelbrot's use of Math.E sad.

I *think* I got all the right places that are callers (or indirect callers) of lazilyImportGlobal, but it's a tricky thing to verify. More eyes on the patch would be great. Preferably some who have memorized the entire repertoire of jsopcode patterns generated by jsemit for accessing global variables!

Comment 12

[Andreas Gal :gal]

Except for the GVAR case we have sprop available in the call site. I feel a bit dirty for not using it and walking all the properties again to re-discover it. Maybe you could split the import function in two, and outer wrapper that finds sprop based on slot, and an inner function that takes sprop directly? Sorry for being such a bean counter here :)

Comment 13

[Graydon Hoare :graydon]

Bean counting is fine; it's why I asked for feedback. But I don't know what you mean about the GVAR case. Most of them don't seem to have sprop available. Is there a function you have in mind? ::record_JSOP_SETGVAR and company for example don't, unless I'm misreading them. The only two I see are ::name() and ::record_SetPropHit (which I'm slightly dubious about the need to modify for this case anyway).

I'm happy to go through an indirect path for those two cases though, if it's all you mean. JSOP_NAME and company are probably pretty hot opcodes.

Comment 14

[Andreas Gal :gal]

*GVAR is pretty rare. Its used for accessing globals inside global script code.  SetPropHit gets called for all property cache hits. Its basically the 2nd part of NAME, but organized as a callback from the interpreter.

Comment 15

[Brendan Eich [:brendan]]

There is no need to search for sprop. The mutating GVAR ops won't be generated if there's a const declaration visible to the program. Two cases:

1. Interactive shell, compiling the shortest compilable set of lines in succession:

const b = 16;

is its own program, no uses, one def, one set. The def opcode, JSOP_DEFCONST, primes the fp->vars slot corresponding to b's index in script->atomMap.vector, but then JSOP_SETCONST updates b by name to initialize it.

The remaining lines are evaluated each as a separate program:

var out = [];
for each (b in [true, "", true, "", true, ""]) out.push(b >> 1);

The for each loop does not execute JSOP_DEFCONST, so its fp->vars slot is null and any assignment to b would use JSOP_BINDNAME/JSOP_SETNAME, not JSOP_SETGVAR. There is no JSOP_FORGVAR, so JSOP_FORNAME is used and it calls js_FindProperty and OBJ_SET_PROPERTY, the latter call finds JSPROP_READONLY set and silently nullifies the mutation.

2. All-lines-one-program: the JSOP_DEFCONST updates fp->vars which is the same vars array in the same fp-addressed frame for the subsequent lines, which helps the emitter to see via the pn_const parse-node member that it should nullify any assignments to b.

There aren't any such assignments in the test for this bug, but you can change the code to force one:

const b = 16;
var out = [];
for each (b in [true, "", true, "", true, ""]) b = 42;
print(b);

I left the for each (b in ...) futile attempt to update b via for-each-in (JSOP_FORNAME) just to minimize change.

So the tracer can pass sprop in from all but the GVAR cases, and pass null from those meaning "no need to check for JSPROP_READONLY".

/be

Comment 16

[Andreas Gal :gal]

Nice. I suggest putting in a comment about this. Import still needs slot so we just pass both and sprop null means don't check.

Comment 17

[Graydon Hoare :graydon]

Attachment: another stab at it

This variant factors out the sprop path to lower the compile-time performance cost, as suggested. Otherwise the same. I did not opt for the approach of "pass null to mean don't check", because "whether the caller has an sprop or only a slot number" is orthogonal to "whether or not the caller intends to write to it", and I dislike entangling meanings like that. It's already hard enough to understand.

Sadly it does not fix bug 465132 anymore -- neither did the previous patch -- because we're now only aborting on import of a global const for writing, and while Math is a global const, Math.PI is a non-global const property modified by incprop. So the first patch bailed when importing Math, period, not Math.PI or Math.E. An additional patch will be required to the path that handles ::prop(), if we want to protect those paths. Or something. It'll be much more work to get right.

Comment 18

[Andreas Gal :gal]

Comment on attachment 350547 [details] [diff] [review]
another stab at it

I suggest a Js_not_reached after the search loop since we must find it. Otherwise looks ok.

Comment 19

[Brendan Eich [:brendan]]

Uh, Math should *not* be const in the global object (not have JSPROP_READONLY). Waldo, I thought you backed that out -- it landed as part of the misbegotten attempt to lock down standard constructors. Please file and fix ASAP!

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 350547 [details] [diff] [review]
another stab at it

>+TraceRecorder::lazilyImportGlobalSlot(unsigned slot, bool forWrite)

I like the layering via overloaded methods, but "forWrite" is confusing if anyone thinks "for" means a for loop of some kind. How about "forWriting"? Or (better IMHO) just "writing"?

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 350547 [details] [diff] [review]
another stab at it

> /* Lazily import a global slot if we don't already have it in the tracker. */
> bool
>-TraceRecorder::lazilyImportGlobalSlot(unsigned slot)
>+TraceRecorder::lazilyImportGlobalSlot(unsigned slot, bool forWrite)
> {
>+    if (forWrite) {
>+        for (JSScopeProperty* sprop = OBJ_SCOPE(globalObj)->lastProp;
>+             sprop; sprop = sprop->parent) {
>+            if (sprop->slot == slot) {
>+                return lazilyImportGlobalProp(sprop, forWrite);
>+            }

Don't overbrace single-line consequents where if (condition) is on one line too.

I still don't like it, sorry. The GVAR optimization can't conflict with JSPROP_READONLY. If you know of such a bug, please file it. The case analysis in comment 15 is not happenstance or something that could change. It's necessary and we ought to be able to count on it in the tracer. I agree with Andreas that a comment and (if you must) DEBUG-only code to assert that the property, if it exists and is being written to, is not read-only, should be enough.

/be

Comment 22

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> Uh, Math should *not* be const in the global object (not have JSPROP_READONLY).
> Waldo, I thought you backed that out -- it landed as part of the misbegotten
> attempt to lock down standard constructors. Please file and fix ASAP!

Already filed as bug 459405; I wish I weren't so far behind on bugmail from the summer, or I'd have seen this much sooner.  :-\

Comment 23

[Graydon Hoare :graydon]

(In reply to comment #21)

> I still don't like it, sorry. The GVAR optimization can't conflict with
> JSPROP_READONLY. If you know of such a bug, please file it. The case analysis
> in comment 15 is not happenstance or something that could change. It's
> necessary and we ought to be able to count on it in the tracer. I agree with
> Andreas that a comment and (if you must) DEBUG-only code to assert that the
> property, if it exists and is being written to, is not read-only, should be
> enough.

Oh, sorry, I wasn't debating the case analysis in the least! It's just that the tracer is -- in the case of ::name() -- reading through the property cache, and even if I extend the property cache helper function here to return sprops when it has them, it's not clear to me from inspecting the property cache code that "calling name() on a global" implies "property cache will have an sprop rather than just a slot number in its cache entry". 

If that's true, great, we can toss the search-for-a-slot loop. If not, well, I can make a fast path for when we have an sprop in name(), but we still need to keep the slow path for when we just have a slot number.

I'm afraid the nature of the property cache is still a little opaque to me, as it's somewhat tied into the semantics of the opcodes that use it.

Comment 24

[Graydon Hoare :graydon]

Attachment: Lower-level and more complete approach

After some discussion with brendan, this variant is somewhat more "two pronged". 

First, it ensures that the property cache always holds sprop cache entries for writes to readonly sprops, rather than slot-number cache entries. 

Second, in the tracer it extends the existing property-cache access helpers to check for the same set of "write to a readonly sprop" conditions and bails from the trace.

This combined approach fixes both the problem in this bug and the Math.PI mutation on trace in bug 465132. And it's smaller and tidier too! What's not to like?

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 353579 [details] [diff] [review]
Lower-level and more complete approach

Looks great, thanks!

/be

Comment 26

[Graydon Hoare :graydon]

Pushed to tracemonkey in revision ed2062a1593c.

Comment 27

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dd786c2ce1ed

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #24)
> First, it ensures that the property cache always holds sprop cache entries for
> writes to readonly sprops, rather than slot-number cache entries.

I should have seen the bug here (noted as blocking bug 473282 only after the fact): js_FillPropertyCache already cached sprop instead of slot for JOF_SET opcodes, but not JOF_INCDEC or JOF_FOR. These flags are disjoint, as they evolved for the decompiler. Perhaps all the mutating bytecodes should have JOF_SET set, but that's for a future cleanup.

The patch here covered JOF_INCDEC and JOF_FOR -- but only if JSPROP_READONLY was also set! Really, we must cache sprop not slot for any mutating op, whether a plain assignment op (JOF_SET), an increment or decrement (JOF_INC | JOF_DEC aka JOF_INCDEC), or a for-in helper (JOF_FOR).

> Second, in the tracer it extends the existing property-cache access helpers to
> check for the same set of "write to a readonly sprop" conditions and bails from
> the trace.

This part's fine, but it may have misled (at least me) in seeming to want a similar (not matching, a warning sign) readonly check in the interpreter. But the interpreter handles readonly mutation attempts differently. The tracer should abort rather than try to generate code to cover the readonly mutation case as the interpreter does, so it needs explicit JSPROP_READONLY tests.

/be

Comment 29

[Bob Clary [:bc] (inactive)]

Checking in js1_6/extensions/regress-465443.js
http://hg.mozilla.org/mozilla-central/rev/708dad34da8f (note typo in bug# in ci comment).

Comment 30

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2