1590694 - Consider adding a const version of HeapPtr

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

It would be nice to have a version of HeapPtr that can't be changed to point to a different target. This would allow developers to document and enforce immutability of class members where appropriate.

The internal pointer will still need to be mutable in case the GC needs to update it, but the wrapper class could disallow mutation but not providing operator=, set methods, etc.

Comment 1

[Jon Coppeard (:jonco)]

It occurred to me that we should make it possible to use | const HeapPtr<T> | for this.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1590694 - Make it possible to use const GC wrappers r=jandem

The main change is to make BarrieredBase::unsafeUnbarrieredForTracing() const and use const_cast to get return the non-const pointer. This makes sense because the GC is allowed to update the pointer used while preseving the logical value of the cell, e.g. when compacting.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1590694 - Use const GCPtrs where possible r?jandem

This replaces GCPtr members with const GCPtr where trival to do so.

Depends on D50618

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1590694 - Use const HeapPtrs where possible r?jimb

This makes HeapPtr members const where it was easy to do so. Flagging jimb for review since most of these were in the debugger.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e42641548cb7
Make it possible to use const GC wrappers r=jandem
https://hg.mozilla.org/integration/autoland/rev/f65f3003e407
Use const GCPtrs where possible r=jandem
https://hg.mozilla.org/integration/autoland/rev/0a2109b4d043
Use const HeapPtrs where possible r=jimb

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3851d4bcbd9d
Backed out 3 changesets for SM bustages on testGCHeapPostBarriers.cpp

Comment 7

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=0a2109b4d043fdca3970c7292ac82ed5279b90c4&selectedJob=273872477&searchStr=Linux%2Cx64%2Cdebug%2CSpidermonkey%2Cbuilds%2Chazard-linux64-shell-haz%2Fdebug%2CSM%28H%29

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=273872477&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/3851d4bcbd9d

[task 2019-10-31T11:23:18.613Z] + NUM_WRITE_HAZARDS=0
[task 2019-10-31T11:23:18.613Z] ++ grep -c '^Function.expected hazard.but none were found' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2019-10-31T11:23:18.614Z] + NUM_MISSING=0
[task 2019-10-31T11:23:18.614Z] + set +x
[task 2019-10-31T11:23:18.614Z] TinderboxPrint: rooting hazards<br/>18
[task 2019-10-31T11:23:18.614Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>82
[task 2019-10-31T11:23:18.614Z] TinderboxPrint: (unnecessary roots)<br/>1022
[task 2019-10-31T11:23:18.614Z] TinderboxPrint: missing expected hazards<br/>0
[task 2019-10-31T11:23:18.614Z] TinderboxPrint: heap write hazards<br/>0
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<JS::Heap<JSObject>, JSObject>, JS::DeletePolicy<TestStruct<JS::Heap<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<const JS::Heap<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<const JS::Heap<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<JS::Heap<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<JS::Heap<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:163
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::GCPtr<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<js::GCPtr<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<const js::GCPtr<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<const js::GCPtr<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::GCPtr<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<js::GCPtr<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:163
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::HeapPtr<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<js::HeapPtr<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<const js::HeapPtr<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<const js::HeapPtr<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::HeapPtr<JSObject*>, JSObject*>, JS::DeletePolicy<TestStruct<js::HeapPtr<JSObject*>, JSObject*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:163
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<JS::Heap<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<JS::Heap<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<const JS::Heap<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<const JS::Heap<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<JS::Heap<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<JS::Heap<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:163
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::GCPtr<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<js::GCPtr<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<const js::GCPtr<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<const js::GCPtr<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::GCPtr<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<js::GCPtr<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:163
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::HeapPtr<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<js::HeapPtr<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<const js::HeapPtr<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<const js::HeapPtr<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:131
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'testStruct' of type 'mozilla::UniquePtr<TestStruct<js::HeapPtr<JSFunction*>, JSFunction*>, JS::DeletePolicy<TestStruct<js::HeapPtr<JSFunction*>, JSFunction*> > >' live across GC call at js/src/jsapi-tests/testGCHeapPostBarriers.cpp:163
[task 2019-10-31T11:23:18.616Z] TEST-UNEXPECTED-FAIL | hazards | 18 rooting hazards detected
[task 2019-10-31T11:23:18.616Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2019-10-31T11:23:18.616Z] + onexit
[task 2019-10-31T11:23:18.616Z] + grab_artifacts /builds/worker/workspace/analysis /builds/worker/artifacts
[task 2019-10-31T11:23:18.616Z] + local analysis_dir
[task 2019-10-31T11:23:18.616Z] + analysis_dir=/builds/worker/workspace/analysis
[task 2019-10-31T11:23:18.616Z] + local artifacts
[task 2019-10-31T11:23:18.616Z] + artifacts=/builds/worker/artifacts
[task 2019-10-31T11:23:18.617Z] + cd /builds/worker/workspace/analysis
[task 2019-10-31T11:23:18.617Z] + ls -lah

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Cosmin Sabou [:CosminS] from comment #7)
I'm not sure why the analysis now detects the hazard here; it's just as much of a hazard as it was before I think?

I'm going to suppress the analysis for these functions because we actually don't want our tests structure to be rooted here. I suppose a more correct approach would be to have it reachable only via a tenured GC thing but I think that's overkill for this test.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfb65a5e10c0
Make it possible to use const GC wrappers r=jandem
https://hg.mozilla.org/integration/autoland/rev/789495222976
Use const GCPtrs where possible r=jandem
https://hg.mozilla.org/integration/autoland/rev/ea95af453bab
Use const HeapPtrs where possible r=jimb

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cfb65a5e10c0
https://hg.mozilla.org/mozilla-central/rev/789495222976
https://hg.mozilla.org/mozilla-central/rev/ea95af453bab