Closed
Bug 1121435
Opened 10 years ago
Closed 10 years ago
Simplify noteMaybeNurseryPtr
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(1 file, 1 obsolete file)
1.40 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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.
Assignee: nobody → jcoppeard
Attachment #8548941 -
Flags: review?(terrence)
Comment 2•10 years ago
|
||
Comment on attachment 8548941 [details] [diff] [review]
bug1121435-note-nursery-ptr
Review of attachment 8548941 [details] [diff] [review]:
-----------------------------------------------------------------
Great!
Attachment #8548941 -
Flags: review?(terrence) → review+
Comment 3•10 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #2)
Looks like I'm wrong because this assertion now fails on Windows.
Updated•10 years ago
|
Attachment #8548941 -
Attachment is obsolete: true
Updated•10 years ago
|
Assignee: jcoppeard → nobody
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Flags: needinfo?(jcoppeard)
Comment 5•10 years ago
|
||
(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.
Flags: needinfo?(jcoppeard)
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8552959 -
Flags: review?(jdemooij)
Comment 8•10 years ago
|
||
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.)
Attachment #8552959 -
Flags: review?(jdemooij) → review+
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
With additional assertion:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72dcb722c0c
Comment 11•10 years ago
|
||
Assignee: nobody → lhansen
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•