1547824 - Value.h refactoring cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P5)

Description

[Iain Ireland [:iain]]

Value.h contains two enums to describe types: JSValueType, and JSValueTag. JSValueType represents the set of types that a Value can have. JSValueTag represents the bit pattern that is used in the tag bits of a Value to indicate its type.

Prior to bug 1401624, there was a direct correspondence between these two enums: a type could be converted into a tag by setting/clearing some bits, and vice versa. In that bug, we changed our Value representation on 64-bit from standard (double-biased) NaN-boxing to object-biased NaN-boxing. As part of that change, we altered the order of JSValueTag. For example, in 32-bit (and the old system), Double and Int32 are the lowest tag values, and all the GC things (Object, String, Symbol, etc...) are the highest tag values. In the new 64-bit system, the lowest tag values are GC things, and the highest tag values are numbers. As a result, to convert between JSValueType and JSValueTag, we now have to use a large switch statement.

We should fix this. I think the cleanest fix is to declare a different order for JSValueType on 32-bit than on 64-bit, to match the order of the tags. This lets both of the implementations use an efficient conversion.

One complication is that there are a handful of places in the code that depend on the ordering of the JSValueType enum values. (Here's a particularly gross example.) Before we can change the order of JSValueType, we have to scrub all those dependencies out of the code.

Comment 1

[Iain Ireland [:iain]]

Repurposing this bug as a more general "make Value.h nicer" bug:

While working on bug 1401624, we moved a bunch of boxing details into the JS::detail namespace. Waldo suggested going a step further:

So it occurs to me, the functions and variables here are just free-floating. And wherever they're used, there's no clarity about which variables are usable at any particular spot. But what if you enclosed all the nunbox32 things in a namespace Nunbox32, and all the punbox64 things in a namespace Punbox64? Then every user would have to so qualify, and it would be very clear at any spot whether the right/a sensible thing was being used. And the boxing-format-agnostic things could still be in JS::detail so it would be clear they did a thing that made sense either way.

Similarly, uses of the JSVAL_TYPE_TO_TAG macro should be removed and converted into JS::detail::ValueTypeToTag calls.

Last, but definitely not least, we should remove the union arms of JS::Value and replace all uses of the payload_ arm with explicit operations on asBits_.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1547824 - Remove js::Value::payload union from 64-bit platforms. r?iain

Use well-defined conversions on the asBits_ instead. The remaining union
arms were not even helpful for debuggers so we remove entirely.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1547824 - Remove js::Value::payload union from 32-bit platforms. r?iain

Use well-defined conversions on the asBits_ instead. This removes union
arms entirely. They are slighly useful for debugging, but we don't have
that on our 64-bit platforms and the gdb helpers can already help us
out.

Depends on D46323

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1547824 - Change JS::Value from union to class. r?jwalden

Now that all the bad type-punning is gone, JS::Value has a single
asBits_ field and we should use a struct aggregate type instead.

Depends on D46324

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1547824 - Add Value::unboxGCPointer helper for JS::Value. r?iain

Abstracts the pointer unboxing pattern for both NUNBOX32 and PUNBOX64
formats. This also encapsulates some of the spectre mitigations.

Depends on D46325

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0188973c2b10
Remove js::Value::payload union from 64-bit platforms. r=iain
https://hg.mozilla.org/integration/autoland/rev/d2df23e104f1
Remove js::Value::payload union from 32-bit platforms. r=iain
https://hg.mozilla.org/integration/autoland/rev/261a25ae16e2
Change JS::Value from union to class. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/f87ca6d25d62
Add Value::unboxGCPointer helper for JS::Value. r=iain

Comment 7

[Raul Gurzau (:RaulG)]

Backed out 4 changesets (bug 1547824) for build bustage at vm/Interpreter.h on a CLOSED TREE.

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=f87ca6d25d62aa443ab07827e725b848690e6fe8&selectedJob=267528739

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267528739&repo=autoland&lineNumber=54139

Log snippet:
[task 2019-09-19T23:38:47.087Z] Wrote explained_hazards.tmp
[task 2019-09-19T23:38:47.087Z] Wrote unnecessary.tmp
[task 2019-09-19T23:38:47.087Z] Wrote refs.tmp
[task 2019-09-19T23:38:47.087Z] Found 61 hazards 251 unsafe references 0 missing
[task 2019-09-19T23:38:47.087Z] Running heapwrites to generate heapWriteHazards.txt
[task 2019-09-19T23:38:47.087Z] PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko' ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyzeHeapWrites.js > heapWriteHazards.txt
[task 2019-09-19T23:39:12.432Z] + check_hazards /builds/worker/workspace/analysis
[task 2019-09-19T23:39:12.432Z] + set +e
[task 2019-09-19T23:39:12.433Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2019-09-19T23:39:12.434Z] + NUM_HAZARDS=61
[task 2019-09-19T23:39:12.435Z] ++ grep -c '^Function.takes unsafe address of unrooted' /builds/worker/workspace/analysis/refs.txt
[task 2019-09-19T23:39:12.436Z] + NUM_UNSAFE=251
[task 2019-09-19T23:39:12.436Z] ++ grep -c '^Function. has unnecessary root' /builds/worker/workspace/analysis/unnecessary.txt
[task 2019-09-19T23:39:12.438Z] + NUM_UNNECESSARY=1201
[task 2019-09-19T23:39:12.438Z] ++ grep -c '^Dropped CFG' /builds/worker/workspace/analysis/build_xgill.log
[task 2019-09-19T23:39:12.466Z] + NUM_DROPPED=0
[task 2019-09-19T23:39:12.467Z] ++ perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' /builds/worker/workspace/analysis/heapWriteHazards.txt
[task 2019-09-19T23:39:12.473Z] + NUM_WRITE_HAZARDS=0
[task 2019-09-19T23:39:12.473Z] ++ grep -c '^Function.*expected hazard.*but none were found' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2019-09-19T23:39:12.474Z] + NUM_MISSING=0
[task 2019-09-19T23:39:12.474Z] + set +x
[task 2019-09-19T23:39:12.474Z] TinderboxPrint: rooting hazards<br/>61
[task 2019-09-19T23:39:12.474Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>251
[task 2019-09-19T23:39:12.474Z] TinderboxPrint: (unnecessary roots)<br/>1201
[task 2019-09-19T23:39:12.474Z] TinderboxPrint: missing expected hazards<br/>0
[task 2019-09-19T23:39:12.474Z] TinderboxPrint: heap write hazards<br/>0
[task 2019-09-19T23:39:12.476Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'args' of type 'js::FixedInvokeArgs<0ul>' live across GC call at js/src/vm/Interpreter.h:93
[task 2019-09-19T23:39:12.476Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'args' of type 'js::FixedInvokeArgs<0ul>' live across GC call at js/src/vm/Interpreter.h:100
[task 2019-09-19T23:39:12.476Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'args' of type 'js::FixedInvokeArgs<1ul>' live across GC call at js/src/vm/Interpreter.h:107
[task 2019-09-19T23:39:12.476Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'args' of type 'js::FixedInvokeArgs<1ul>' live across GC call at js/src/vm/Interpreter.h:115
[task 2019-09-19T23:39:12.476Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'args' of type 'js::FixedInvokeArgs<2ul>' live across GC call at js/src/vm/Interpreter.h:123
[task 2019-09-19T23:39:12.476Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'args' of type 'js::FixedInvokeArgs<2ul>' live across GC call at js/src/vm/Interpreter.h:132

Comment 8

[Ted Campbell [:tcampbell]]

We've tracked failure back to a bug in the hazard analysis. Once that is fixed these patches can be landed again.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd93d5eedd58
Remove js::Value::payload union from 64-bit platforms. r=iain
https://hg.mozilla.org/integration/autoland/rev/9fc20300934b
Remove js::Value::payload union from 32-bit platforms. r=iain
https://hg.mozilla.org/integration/autoland/rev/0f6fd838cc86
Change JS::Value from union to class. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/71e6b685cf60
Add Value::unboxGCPointer helper for JS::Value. r=iain

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/bd93d5eedd58
https://hg.mozilla.org/mozilla-central/rev/9fc20300934b
https://hg.mozilla.org/mozilla-central/rev/0f6fd838cc86
https://hg.mozilla.org/mozilla-central/rev/71e6b685cf60

Comment 11

[Ted Campbell [:tcampbell]]

The changes seem to have stuck so I'll close up this bug.