1121435 - Simplify noteMaybeNurseryPtr

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

A comment in noteMaybeNurseryPtr in Assembler-shared.h suggests that there is complexity there that is due to PJS.  That complexity can be removed now, but it's not 100% clear what it is.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1121435-note-nursery-ptr

Previously this could be called off-main thread by PJS, although never with a nursery pointer.  Now that PJS is gone we can assert that this is never called off main thread at all.  This gives us a stronger assertion that nothing off main thread can touch nursery pointers.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8548941 [details] [diff] [review]
bug1121435-note-nursery-ptr

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

Great!

Comment 3

[Jon Coppeard (:jonco)]

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

Looks like I'm wrong because this assertion now fails on Windows.

Comment 4

[Lars T Hansen [:lth]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> (In reply to Terrence Cole [:terrence] from comment #2)
> 
> Looks like I'm wrong because this assertion now fails on Windows.

Jon, do you have any more context than that?  On what test it failed, 32-bit, 64-bit?

Absent any more information I'm likely to just change the comment to note that the assertion only holds if the pointer is indeed a nursery pointer, and leave it at that.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Lars T Hansen [:lth] from comment #4)
32-bit build, in the package stage:

https://treeherder.mozilla.org/logviewer.html#?job_id=4249168&repo=try

There's no backtrace unfortunately.

Comment 6

[Jon Coppeard (:jonco)]

These ImmMaybeNurseryPtr come from IonCaches.cpp.  I don't know enough about how this works to say whether we should be seeing this type in off-main thread compilation.  I guess the comment may just be out of date with regards to PJS being the only user.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Fix the comment to reflect reality

I too think the comment is stale.  noteMaybeNurseryPointer is reachable from eg cmpPtr in the x64 assembler, which is reachable from eg visitCompareAndBranch in the x64 code generator, which is reachable from the rest of the compiler and should thus be possible to reach during off-thread compilation.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8552959 [details] [diff] [review]
Fix the comment to reflect reality

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

r=me with nit below addressed.

::: js/src/jit/shared/Assembler-shared.h
@@ +934,1 @@
>              MOZ_ASSERT(GetJitContext()->runtime->onMainThread());

Please add another assert that fails if we're Ion-compiling on the main thread. For fuzzing with --no-threads/--ion-offthread-compile=off it'd be useful to catch that as well:

  MOZ_ASSERT(!GetJitContext()->runtime->mainThread()->ionCompiling);

(Ion compilation shouldn't use nursery pointers on the main thread either.)

Comment 9

[Jan de Mooij [:jandem]]

The problem is probably pushValue(const Value &val) in the x86 and ARM macro assemblers, it uses ImmMaybeNurseryPtr if val.isMarkable(). We could probably fix that somehow but I think those 2 asserts are good enough for now.

https://dxr.mozilla.org/mozilla-central/search?q=ImmMaybeNurseryPtr

Comment 10

[Lars T Hansen [:lth]]

With additional assertion:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72dcb722c0c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a72dcb722c0c