Closed Bug 779390 Opened 12 years ago Closed 12 years ago

IonMonkey: Assertion failure: addr % Cell::CellSize == 0, at ../../gc/Heap.h:840

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
firefox17 --- unaffected
firefox18 --- verified
firefox19 --- verified
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: dvander)

References

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [ion:p1:fx18] [jsbugmon:verify-branch=mozilla-aurora;mozilla-beta,ignore])

Attachments

(1 file)

The following testcase asserts on ionmonkey revision b46621aba6fd (run with --ion -n -m --ion-eager -a):


function TestCase(n, d, e, a) {
  this.passed = getTestCaseResult(e, a);
}
function getTestCaseResult(expected, actual) {}
try {
for (var i = 0; i < bomchars.length; i++)
  try {} catch(ex) {}
} catch(exc0) {}
["a"].map(function(s) {gczeal(4);})[0]
new TestCase();
new TestCase("", "", 5e-324, i);
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
MarkThingOrValueRoot can only be used if we know that a Value-typed slot is definitely traceable. That's the case when embedding Values in the code stream but not for values stored on the stack at safepoints. Luckily I think fixing this will just remove #ifdef's in a few places.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attached patch fixSplinter Review
I honestly don't know how we got away with this for so long. 

This patch does the following things:

 * MarkThingOrValue is gone, since almost every called was wrong. It has been
   re-inlined into its one correct caller.
 * MarkGCThing now has a Root and Unbarriered variant.
 * SafepointWriter now holds all of the encoding logic, so CodeGenerator is
   no longer responsible for carefully using an awkward API.

And finally, the fix: X64 safepoints now have "value registers" that are distinct from "gc registers". In addition it also uses "value slots" which were previously only used by x86/ARM.
Attachment #648183 - Flags: review?(nicolas.b.pierron)
Comment on attachment 648183 [details] [diff] [review]
fix

Review of attachment 648183 [details] [diff] [review]:
-----------------------------------------------------------------

Good!

This patch specialize the marking functions by encoding more type information in the safepoint.

The only thing I am not sure to understand is what in MarkThingOrValueRoot — what input — was causing the previous error and could it happen in TraceDataRelocations ?

::: js/src/ion/LinearScan.cpp
@@ +1228,5 @@
> +                if (a->isGeneralReg() && !ins->isCall()) {
> +#ifdef JS_PUNBOX64
> +                    if (reg->type() == LDefinition::BOX) {
> +                        safepoint->addValueRegister(a->toGeneralReg()->reg());
> +                    } else

nit: remove { } ?

::: js/src/ion/Safepoints.cpp
@@ +76,1 @@
>              IonSpew(IonSpew_Safepoints, "    %s reg: %s", type, (*iter).name());

Great, so we would be able to monitor how costly are the extra spills.
Would be great to add "volatile" by masking against the Registers::WrapperMask mask, such as we can easily estimate the overhead of spilling everything.

At the same time it would be nice to check if it is worth keeping the condition on the spilled registers.

@@ +256,5 @@
> +#endif
> +
> +void
> +SafepointWriter::encode(LSafepoint *safepoint)
> +{

Great ! I like to see that under the SafepointWriter as you did to keep it symmetric with the Snapshot reader.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +649,5 @@
>          size_t offset = reader.readUnsigned();
>          InstructionIterator iter((Instruction*)(buffer+offset));
>          const void *ptr = js::ion::Assembler::getPtr32Target(&iter);
>          // No barrier needed since these are constants.
> +        gc::MarkGCThingUnbarriered(trc, reinterpret_cast<void **>(&ptr), "immgcptr");

It would be nice to keep the "ion-" prefix for all Ion related marking, such as we can trace&blame more easily what is accounted by IonMonkey and from where it is coming from.

::: js/src/ion/shared/Assembler-x86-shared.cpp
@@ +39,5 @@
> +        if (*word >> JSVAL_TAG_SHIFT) {
> +            jsval_layout layout;
> +            layout.asBits = *word;
> +            Value v = IMPL_TO_JSVAL(layout);
> +            gc::MarkValueUnbarriered(trc, &v, "masm-value");

nit: Keep a consistent marking name across all TraceDataRelocations.

@@ +49,2 @@
>          // No barrier needed since these are constants.
> +        gc::MarkGCThingUnbarriered(trc, reinterpret_cast<void **>(ptr), "masm-ptr");

nit: Same here.
Attachment #648183 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/88d3097f006d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
Flags: in-testsuite?
Keywords: sec-high
How can I debug a ion monkey build to see the assertion ?
I'm trying to reproduce the issue on a normal ion monkey build from 2012-07-31.
Moreover, when running with "--ion -n -m --ion-eager -a" I get: "Error: argument -a requires an application name"
Thanks.
(In reply to Paul Silaghi [QA] from comment #9)
> How can I debug a ion monkey build to see the assertion ?
> I'm trying to reproduce the issue on a normal ion monkey build from
> 2012-07-31.

You need to build a JS shell, a Firefox build will not work for this. But since recently, we should be able to verify some of these bugs automatically. Judging from the flags, I assume aurora and beta need verification? Trying to have these automatically verified (although the bot doesn't support reading or setting status flags yet).
Whiteboard: [jsbugmon:update][ion:p1:fx18] → [jsbugmon:verify-branch=mozilla-aurora,mozilla-beta][ion:p1:fx18]
Whiteboard: [jsbugmon:verify-branch=mozilla-aurora,mozilla-beta][ion:p1:fx18] → [jsbugmon:verify-branch=mozilla-aurora;mozilla-beta][ion:p1:fx18]
Whiteboard: [jsbugmon:verify-branch=mozilla-aurora;mozilla-beta][ion:p1:fx18] → [ion:p1:fx18] [jsbugmon:verify-branch=mozilla-aurora;mozilla-beta,ignore]
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-aurora (tried revision 376035760ee0).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-beta (tried revision 3040211d5324).
Marking verified based on comment 11. Note that the verification mechanism did reproduce the issue on the ionmonkey branch but it did not on the older versions of the aurora and beta branches. The options and test should work the same way though so I'd consider this verification sufficient for this bug.
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.