Test case: function f() { var x = 10; var g = function(p) { for (var i = 0; i < 10; ++i) x = p + i; } for (var i = 0; i < 10; ++i) { g(100 * i + x); } print(x); } f(); host-7-8:debug dmandelin$ jsd /tmp/ttt.js 4600 host-7-8:debug dmandelin$ jsd -j /tmp/ttt.js 128 The cause: when we call |setCallProp| to record setting x, we see that the frame is not in range, so we generate LIR to call js_SetCallVar. That works if there are no nested traces. But if later we have an outer trace call this trace, the call to js_SetCallVar still updates the call object properly, but doesn't update the outer trace native stack variable, which is the "real" copy. As the "real" copy, when we leave trace, the (not-set) native stack variable is copied into the synthesized frame, so we get the wrong value.

Patch made with lots of help from dvander that fixes this test case and doesn't regress trace-tests. There are still some issues we want to check on.

the new import code in attemptTreeCall() technically works but mostly by accident. clearFrameSlotsFromTracker() wipes out all of |fp.slots|. if |callDepth > 0|, this will take away slots that are the inner function's arguments. yet, it still works because we repopulated |importTypeMap| earlier. repopulating this shouldn't be necessary and it was masking the tracker problem. the fix (i believe) is to only wipe out entry frame slots that could be referenced by upvars - arguments and fixed slots. the attached patch applies on top of dmandelin's patch and: - removes the repopulation of importTypeMap - clears only argv/nfixed slots from the entry frame (ugh) - adds some debug code to make sure the NESTED_EXIT snapshot doesn't capture any TT_INGORE slots.

This combines the other two, refactors a bit, and adds a test case.

Comment on attachment 419443 [details] [diff] [review] Patch 2 >+ // Only fixed slots are imported, so clear only those. This is not entirely true, we can initially import extra slots on the stack. It's just that here we don't want to clear them. Maybe, "only clear slots which might be referenced via upvars" (or "not owned by the down frame", but that's not entirely true either since we don't clear ANY sp slots). >+ clearFrameSlotsFromTracker(which, fp, fp->script->nfixed); >+} Nice refactoring, my previous "ugh" was because I had resorted to code duplication. >+ LIns *fp_ins = lir->insLoad(LIR_ldp, cx_ins, offsetof(JSContext, fp)); >+ LIns *fpcallobj_ins = lir->insLoad(LIR_ldp, fp_ins, offsetof(JSStackFrame, callobj)); >+ LIns *br1 = lir->insBranch(LIR_jf, lir->ins2(LIR_eq, fpcallobj_ins, callobj_ins), NULL); LIR_eq -> LIR_peq >+ LIns *frameInfo_ins = lir->insLoad(LIR_ldp, callstackBase_ins, 0); >+ LIns *typemap_ins = lir->ins2(LIR_addp, frameInfo_ins, INS_CONST(sizeof(FrameInfo))); INS_CONST -> INS_CONSTWORD >+ LIns *type_ins = lir->insLoad(LIR_ldcb, lir->ins2(LIR_addp, typemap_ins, slot_ins), 0); slot_ins has to be sign-extended on 64-bit, so lir->ins_u2p(slot_ins) (nop on x86). >+ guard(true, >+ addName(lir->ins2(LIR_eq, type_ins, lir->insImm(getCoercedType(v))), I think this should be determineSlotType(). If it's an undemotable int, getCoercedType will still return TT_INT32. The outer tree could want an integer and the type check would succeed, but the code below would write back a float because isPromoteInt() would return false. >+ lir->insStorei(storeValue_ins, lir->ins2(LIR_addp, stackBase_ins, offset_ins), 0); offset_ins needs a lir->ins_u2p() r=me with those changed

Backed out due to mochitest failures.

Fix for the bustage of Patch 2, currently running on try server. The problem was that we used determineSlotType, but the pointer we provided to it was bogus. Even if it were correct, there would still be a problem because the value is not guaranteed to be tracked, which is required by determineSlotType. The new version has the same logic as determineSlotType but works on an untracked value.

This was nominated for blocking 1.9.2 - can I get a description of the expected effect on web compatibility that led to that nomination, as well as how long this bug has existed on mozilla1.9.2?

https://bugzilla.mozilla.org/show_bug.cgi?id=536943#c1 has a 1.9.2 regression range, 2009-12-20 is the first broken build. Trunk 2009-12-05 is the first broken build.

I nom'd this because it affects web compat in bug 536943 (jsbeautifier.org not working correctly). Bug 532568 introduced that problem, although I think the bug was actually latent before then.

Enough has changed that I think I need an official r+ before trying to land this again.

It looks like this is crashing the v8 test machines.

(In reply to comment #14) > It looks like this is crashing the v8 test machines. Checking.

Output is different w/ or w/o JIT turned on. Also, if I comment out the line that calls |clearEntryFrameSlots| in jstracer.cpp, it works correctly with JIT on. I don't fully understand the problem yet, but I have most of the elements. In this test, we set the variable j, then call an inner trace, then read j. * Setting j. Because we read from an array, |xa[i]| is expressed as a js_UnboxDouble. This means it is represented as a double on trace, and thus so is |i + xa[i]|. This also means |j| is now a double and is not a |promoteInt|. * Calling g. * In the unregressed version, calling |g| does nothing much. * In the regressed version, we also clear the tracker slots for |f|, in case |g| was an inner function of |f| and modified any upvars. This means we will have to import |j| if we ever happen to use it later. In order to enable that, at this point we recompute the typemap for the parts of |f|. Because we don't have the LIR instruction for |j| any more (the whole idea is that |j| may have been set in the inner trace), we can only do this by looking at the value, which we will conclude can be represented as an int, and so |j| is set to be an int in the import typemap. * Getting j. * In the unregressed version, we get |j| as a double and everything works. * In the regressed version, we get |j| as an int and get the wrong value. The failure mode of the v8 test is that crypto.js iloops (run-crypto.js will also show this in the shell), so I think it's probably another wrong value bug like this. I believe the solution is to revive some code that was in an earlier version of this patch that updates the import typemap just before an inner tree call. At this point, the LIR that computes the values of the stack slots is available, so we have all the promoteInt information. We have guards that ensure the inner tree will not change any types of native stack slots by setting upvars. So, when the call is done, the type of the value in the native stack slot will match the import typemap and things should work correctly. With |Patch 4|, where we don't update the import type map in that way, we are using the original import type map, based on the jsvals on the stack when we started recording, so it says 'int' for anything that can be represented as one, even if it actually changed later. Before any of these patches, we would never have to reimport a variable after it has been set, so the staleness of the import type map didn't matter.

Thanks for the explanation: this blocks.

Fix per my last comment. This passes all of our standard shell tests and runs v8 OK.

Comment on attachment 419710 [details] [diff] [review] Patch 5 (fix stale import typemap problem) >+ /* >+ * Fill in everything below the current frame so the tracker can re-import >+ * all values safely. The inner tree CANNOT have touched the types of any >+ * variables above the current frame (i.e. upvars). >+ */ >+ importTypeMap.setLength(NativeStackSlots(cx, callDepth)); >+ DetermineTypesVisitor visitor(*this, importTypeMap.data()); >+ VisitStackSlots(visitor, cx, callDepth); >+ This I think deserves an extra comment, though just a reference to this bug # and comment would suffice. determineSlotType() can read from the importTypeMap, which at first glance might seem to be really bad here. But it's safe because there will always be tracker values for which there are no importTypeMap entries. If there isn't a tracker value, that importTypeMap slot will be valid, and the store to that slot will just be a read from the same slot. r=me

Currently working on the 1.9.2 patch. The bug appears to be related to this line of code in emitTreeCall: import(ti, inner_sp_ins, exit->numStackSlots, fullMap.length() - exit->numStackSlots, exit->calldepth, fullMap.data()); For a simple call, exit->calldepth is 0, so we reimport just the current stack frame, and not the entry stack frame. I assume that in general, exit->calldepth is the call depth at the inner tree exit relative to the inner tree entry point, so we are reimporting exactly the frames that are directly part of the inner tree.

Can we expedite review and landing here, please?

It looks like this fix fixed a hang I was seeing with jsbeautifier.org. That hang was introduced by the followup fix for bug 532823, as far as I can see.

(In reply to comment #25) > It looks like this fix fixed a hang I was seeing with jsbeautifier.org. That > hang was introduced by the followup fix for bug 532823, as far as I can see. Yes, that hang was filed as bug 536943, which I dup'd to this one after deciding this bug was almost certainly the root cause of that one.