The following testcase crashes on mozilla-central built with --enable-exact-rooting --enable-gcgenerational, revision 1ad9af3a2ab8 (run with --fuzzing-safe --ion-eager): var lfcode = new Array(); var e = 2; lfcode.push("\ var optionNames = options().split(',');\ for (var i = 0; i < optionNames.length; i++) {}\ "); lfcode.push("gczeal(7,e);"); lfcode.push("4"); lfcode.push("\ var S = new Array();\ var item = 0;\ var limit = 0x0061;\ for ( var i = 0x007A; i >= limit; i-- ) {\ S[item] += ' ';\ S[item] += ',';\ }\ eval(S);\ "); var lfRunTypeId = -1; while (true) { var file = lfcode.shift(); if (file == undefined) { break; } loadFile(file) } function loadFile(lfVarx) { if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) { switch (lfRunTypeId) { case 4: eval("(function() { " + lfVarx + " })();"); break; default: evaluate(lfVarx, { noScriptRval : true }); break; } } else if (!isNaN(lfVarx)) { lfRunTypeId = parseInt(lfVarx); } }

Crash trace: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7fd5cf5 in ?? () (gdb) bt #0 0x00007ffff7fd5cf5 in ?? () #1 0x0000000000000000 in ?? () (gdb) x /i $pc => 0x7ffff7fd5cf5: mov %r11,(%r9,%r8,8) (gdb) info reg r8 r9 r11 r8 0x0 0 r9 0xfffafffff545c140 -1407375063531200 r11 0xfffafffff545c180 -1407375063531136 Jonco, this is the crash I was talking about earlier, I finally managed to get a working test.

Reproduced but requires --ion-parallel-compile=off

This is crashing while trying to do a StoreElementT instruction following a Concat instruction. The problem seems to be that the what the code expects to be a pointer to array elements for the store is actually a value containing a string.

I've looked into this a bit further and it seems that an LConcat instruction is trashing r8 when a minor GC happens.

The problem is that we forward slots/element pointers in Ion stack frames too soon, allowing the possibility that the slots have not yet been tenured. There's no check in the nursery that this happens and we merrily overwrite a slot pointer with a Value. The attached patch fixes this by moving the forwarding out of the normal Ion frame marking and doing it after collectToFixedPoint() has happened. There are a couple of rough edges. I wanted to add an assertion in forwardBufferPointer() to check that we have a valid pointer to malloc'd memory rather than a Value, so currently I'm checking we can read and write the first word back to it, which is kind of gross. Maybe it would be better to reserve a word before the start of the buffer in debug builds, but this seems like it could overcomplicate things. For safepoints, it seems I have to iterate through the various other types of slots to get to the "slots or elements slots", which isn't ideal. There may be a better way of doing this.

Comment on attachment 8359294 [details] [diff] [review] bug950927-ion-elements-ptr Review of attachment 8359294 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=me (In reply to Jon Coppeard (:jonco) from comment #5) > Created attachment 8359294 [details] [diff] [review] > bug950927-ion-elements-ptr > > The problem is that we forward slots/element pointers in Ion stack frames > too soon, allowing the possibility that the slots have not yet been tenured. > There's no check in the nursery that this happens and we merrily overwrite a > slot pointer with a Value. I was never 100% convinced that this case was impossible; figured the fuzzers would find it much sooner. > The attached patch fixes this by moving the forwarding out of the normal Ion > frame marking and doing it after collectToFixedPoint() has happened. Yup, that's the right solution. > There are a couple of rough edges. I wanted to add an assertion in > forwardBufferPointer() to check that we have a valid pointer to malloc'd > memory rather than a Value, so currently I'm checking we can read and write > the first word back to it, which is kind of gross. Maybe it would be better > to reserve a word before the start of the buffer in debug builds, but this > seems like it could overcomplicate things. Clever! Wish I had thought of doing that when I implemented this originally. Initially, we had lots of complex DEBUG-only infrastructure (e.g. the nursery was fully iterable), but we decided to rip most all of that out to make the JIT implementation work easier. I think this falls into the same category. > For safepoints, it seems I have to iterate through the various other types > of slots to get to the "slots or elements slots", which isn't ideal. There > may be a better way of doing this. Yeah, that's going to be quite slow. Could you check how much time we're actually spending doing that iteration on octane? If it's significant, it should be pretty trivial to write the locations for rewriting to an optional vector arg for post-processing.

I ran some timings with the v8 benchmarks and with mochitest-bc: the results were that we spend 1.37% and 0.07% of the time in this new phase respectively. So it's something we could potentially improve for the benchmarks but I don't think it's a big cause for concern right now.