907135 - Assertion failure: false (MOZ_ASSUME_UNREACHABLE(Modified registers between VM call and OsiPoint)), at jit/shared/CodeGenerator-shared.cpp:521

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision b7f636fada9f (run with --fuzzing-safe --ion-eager), built with rooting analysis enabled:


gczeal(6);
enableOsiPointRegisterChecks();
function DiagModule(stdlib, foreign) {
    var test = foreign.test;
    var x = 0.0;
    while(1) test(1, x);
}
var foreign = {
  test:function(a,b) {
    unescape(a+":"+b)
    var c = [0.0];
  }
}
var fast = DiagModule(this, foreign);

Comment 1

[Jon Coppeard (:jonco) (PTO until 14th September)]

Attachment: bug907135-rootingAssertion

This needs a SkipRoot in getNewType() to stop the rooting analysis poisoning the hash value.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 792939 [details] [diff] [review]
bug907135-rootingAssertion

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

r=me

Comment 3

[Jon Coppeard (:jonco) (PTO until 14th September)]

(In reply to Terrence Cole [:terrence] from comment #2)

Duh, I attached the patch to the wrong bug.  This is the fix for bug 907147.

I actually couldn't get this one to reproduce for me when I tried it.

Comment 4

[Christian Holler (:decoder)]

Did you try on x86 (32 bit) Linux with the configuration options --enable-debug --enable-optimize --enable-valgrind --disable-threadsafe --enable-root-analysis?

If this doesn't repro for you, I can try to get you another test.

Comment 5

[Jon Coppeard (:jonco) (PTO until 14th September)]

(In reply to Christian Holler (:decoder) from comment #4)

Cheers, I've managed to repro this now - I may not have been using an optimized build before.

Comment 6

[Terrence Cole [:terrence]]

Excellent. I was not able to reproduce either this or bug 907147 yesterday.

I was using:

CC="gcc -m32" CXX="g++ -m32" ./configure --enable-optimize --enable-debug --target=i686-linux-gnu --disable-threadsafe --enable-root-analysis --enable-valgrind --enable-gczeal --enable-more-deterministic --with-system-nspr

I guess it's --enable-more-deterministic that is the issue. Now that Jon has it reproducing, I'll let him take it.

Comment 7

[Jon Coppeard (:jonco) (PTO until 14th September)]

It looks the rooting analysis is poisoning the contents of a safepoint, and when this is compared against the current register state by verifyOsiPointRegs() it is found to be different.

The safepoint is on the stack, but not in a region indicated as part of the JIT stack by JitActivationIterator.  I haven't worked out how this is possible yet.

Some debug output:

CheckStackRootsRangeAndSkipJit
  skip jit stack at 0xffffc9c4 to 0xffffcaf4
  check range 0xffffc810 to 0xffffc9c4
  skip jit stack at 0xffffd18c to 0xffffd264
  check range 0xffffcaf4 to 0xffffcc64

Expected value: 0xf7842c30
Actual value:	0xda842c30 loaded from 0xffffcba8

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Jon Coppeard (:jonco) from comment #7)
> It looks the rooting analysis is poisoning the contents of a safepoint, and
> when this is compared against the current register state by
> verifyOsiPointRegs() it is found to be different.
> 
> The safepoint is on the stack, but not in a region indicated as part of the
> JIT stack by JitActivationIterator.  I haven't worked out how this is
> possible yet.

This check is supposed to check if the content remains identical between the call and the exit of one instruction (checked at the OSI point).  We are supposed to disable this check by setting a flag on the JitActivation when a GC happen, to avoid having cases where a moving GC change the address of something, knowing that this dumped vector is not marked.

> Some debug output:
> 
> CheckStackRootsRangeAndSkipJit
>   skip jit stack at 0xffffc9c4 to 0xffffcaf4
>   check range 0xffffc810 to 0xffffc9c4
>   skip jit stack at 0xffffd18c to 0xffffd264
>   check range 0xffffcaf4 to 0xffffcc64
> 
> Expected value: 0xf7842c30
> Actual value:	0xda842c30 loaded from 0xffffcba8

How does the poisoning works?  Is that for conservatively scan pointers which looks like
dead pointers?

Comment 9

[Jon Coppeard (:jonco) (PTO until 14th September)]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)

> How does the poisoning works?  Is that for conservatively scan pointers
> which looks like dead pointers?

We scan the stack for things that look like pointers, and then check whether there is an active root for that pointer.  If not, we poison it by setting the top byte to 0xda.

It sounds like we want to either skip the dumped data, or possibly disable this check entirely if rooting analysis is active.

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Jon Coppeard (:jonco) from comment #9)
> (In reply to Nicolas B. Pierron [:nbp] from comment #8)
> 
> > How does the poisoning works?  Is that for conservatively scan pointers
> > which looks like dead pointers?
> 
> We scan the stack for things that look like pointers, and then check whether
> there is an active root for that pointer.  If not, we poison it by setting
> the top byte to 0xda.

This means that if we root it once, all the non rooted copy would be poisoned, right?

> It sounds like we want to either skip the dumped data, or possibly disable
> this check entirely if rooting analysis is active.

Then, yes, we should disable these assertions during the rooting analysis.

Comment 11

[Jon Coppeard (:jonco) (PTO until 14th September)]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)

> This means that if we root it once, all the non rooted copy would be
> poisoned, right?

The analysis will poison all non-rooted things on the stack, yes.

> Then, yes, we should disable these assertions during the rooting analysis.

Ok great.  To avoid rebuilding jit code with/without the assertions when enabling/disabling rooting analysis at runtime, I think it would be simplest just to not include these assertions in a rooting analysis build.

Comment 12

[Jon Coppeard (:jonco) (PTO until 14th September)]

Attachment: bug907135-disableOsiCheck

Patch to disable the OSI register checking in rooting analysis builds.

Comment 13

[Jon Coppeard (:jonco) (PTO until 14th September)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound

Comment 14

[Jon Coppeard (:jonco) (PTO until 14th September)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42c942b5eada

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42c942b5eada