1627248 - Use placement new to forward cells to work around undefined behaviour

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Use of RelocationOverlay::next_ to hold a linked list pointer is undefined behaviour because the underlying type of that location is something else. We can get around this by using placement new to start a new lifetime for the cell memory as a RelocationOverlay before using it as such. (I was surprised to discover that destruction of the original contents of the memory is not required first).

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1627248 - Refactor cell methods that read cell header flags r?tcampbell

Depends on D69524

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1627248 - Use placement new when forwarding cells r?tcampbell

Depends on D69575

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1627248 - Check GC thing classes have standard layout type r?tcampbell

I wanted to assert that the most derived type of all GC things had standard layout type; sadly this was not true. The restrictions are quite strong, and I don't think this is realistic. However we can assert that the base GC thing type meets the restrictions with one minor change and I think that's most of the way there.

Depends on D69575

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6ca42417f92
Refactor cell methods that read cell header flags r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/cc6e408e477c
Check GC thing classes are have standard layout type r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/f7bda197d6b9
Use placement new when forwarding cells r=tcampbell

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5d3f290d892
Backed out 3 changesets for causing bustages in src/js/src/gc/GC.cpp

Comment 6

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (Bug 1627248) for causing bustages in src/js/src/gc/GC.cpp

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=676749&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&tochange=a5d3f290d892c7d62db74d1a18433f1f9025d146&fromchange=f7bda197d6b97e098384bfeb1086a5399a468de6&selectedJob=296566540

Failure log:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296566525&repo=autoland&lineNumber=9183
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296566603&repo=autoland&lineNumber=4151

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae8829b46a23
Refactor cell methods that read cell header flags r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/6b6f3e10c9a4
Check GC thing classes have standard layout type r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/5bbd86857de9
Use placement new when forwarding cells r=tcampbell

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ae8829b46a23
https://hg.mozilla.org/mozilla-central/rev/6b6f3e10c9a4
https://hg.mozilla.org/mozilla-central/rev/5bbd86857de9