Updated patch that applies to tip. Passes with-ion jit-tests, baseline-only jit-tests, and interpreter-only jit-tests.

Testing against '--baseline-eager --no-ion' and '--ion-eager' right now. Will run for green on try (x64 arch only), and then we should be ready for fuzzing.

Still need to run on try and add arch support for arm64 and mips64, but that can be worked on in parallel with fuzzing on x64.

Try run for latest patch seems to be green-ish on x64: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04df9197f5861056a9940c68818212ebdc92d55e

Rebased object-biased nanboxing patch that applies cleanly to m-i and m-c tip.

Updated patch, but with the enum-define-hack removed. This is causing build problems for decoder on fuzzing infra.

Synopsis of problem:

This patch changes the JSValueTag constants from large (17-bit values representable only with uint32_ts) to small (<8-bit values).

The JS_ENUM_BEGIN hack in Value.h was skipping the type info for the struct definition, and passing 'packed' instead. For the previous constants this would generate a uint32_t type. The new constants generate a uint8_t, I presume.

This breaks on some compilers, presumably wherever MOZ_IS_GCC is defined.

Similar crash to the last comment, but without import:

gczeal(8, 4);
var ProxiedConstructor = new Proxy(Intl.PluralRules, new Proxy({}, {}));
obj = Reflect.construct(Intl.PluralRules, [], ProxiedConstructor);

Found the issue. Just documenting for posterity - there's a boxed null-ptr Value being created in a (rooted) AutoValueArray on the C stack (within some code for calling into FinishDynamicModuleImport).

With acknowledgements to rr, the culprit was traced back to here:

https://searchfox.org/mozilla-central/source/js/src/jsapi.h#73

Rolling updated patch.

Updated patch. Passes both the failing test cases posted so far.

Also rebased to apply to tip (there were some conflicting changes in a wasm file).

Running on try against m-i tip: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f038d190e5393653fde3957fffd9d2e8b6c11d3

(In reply to Kannan Vijayan [:djvj] from comment #99)

Also rebased to apply to tip (there were some conflicting changes in a wasm file).

Please specify the exact m-c rev hash in the future, "tip" can still mean different hashes and entail a bunch of guesswork. Thanks!

Updated patch. This should apply to the current m-c revision c849fb69e2e7.

I only got it to apply on m-c rev b482c6618d72.

I'd like a rebase on current m-c, that m-c rev has bug 1526279 throwing constantly (since fixed on m-c). Thanks!

Cool. Let me rebase and re-post the main patch after I post the arm64 delta. Just got that to build (simulator at least).

Main patch rebased to tip. Should apply to m-c revision 'bf3951daded0'.

I found something that might be related in bug 1529377 comment 2 so please take a look there.

Thanks for the heads up gary.

Commented on the other bug but will repeat here for posterity - the ARM64 build doesn't work yet. I haven't gotten it to pass tests yet, but will do so soon.

For now, the fuzzing with this patch should be restricted to x86-64 archs. This is where we expect to have a correct implementation right now, and a large chunk of the code involved is arch-independent (the first early fuzzbug found on this was cross-arch, for example).

If we can get x86-64 + cross-arch code fuzzed while I get ARM64 stood up generally (or at least no regressions from wherever sstangl/nbp are at), we should be able to fuzz the delta for arm64 more quickly (the differences should simply be in the core macroassembler code).

Thanks, gary!

Repinging :decoder to make sure he's aware of the latest patch and can carry on from the light testing. I sort of assumed from the last post I made, but I realize no I didn't explicitly put a needinfo on it.

Rebased main patch.

Delta patch for ARM64 support rebased and one fix.

Iain: so I'm pinging you finally. Test failures on ARM64 are still pretty heavy and happening in TI. The x86-64 patch works ok. If you can help out, it would be much appreciated.

The arm64 patch is a delta patch, so to build you want to apply the main-x64 patch prior. Should apply clean to m-i tip.

Thanks again!

Sounds good. Taking a look.

Ok, this arm64 patch passes all tests on all modes (timeouts excluded), with the simulator.

Gkw & Decoder, when you guys have the time can you fuzz this delta patch (on top of the nanbox-main-x64.patch) on ARM64?

Time to send this for review.

Updated arm64 patch. This takes a slightly cruder approach to doing unboxDouble() (unboxing in place, writing to destination, reboxing in place), which avoids the scratchreg and register exhaustion issue.

:decoder - no need to test this one yet - let's fix the fuzzbugs found and then I'll ping you again.

This seems to fix fuzzbugs in comments 119 and 120. Other still failing.

Rebased main patch to tip.

Reposting arm64 patch to maintain ordering in list of attachments.

I took another shot at this. I'm now confused about why we thought the previous patches passed jit-tests, but with the attached patch (plus the two above) everything works, including decoder's fuzz bugs.

I've done a semi-rigorous audit to make sure that all of the x64 changes have an arm64 equivalent, so I feel pretty good about this version.

When boxing changes get landed, we should fix [1] to just use UndefinedValue() instead of it's weird 0x0 optimization.

[1] https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/js/src/vm/JSFunction-inl.h#144

I believe this all needs to move to phabricator as well to be reviewable

One more patch to clear up a number of build problems on try.

1. As mentioned above, we need to keep the old Value(double) constructor for 32-bit.
2. We were doing dumb things with macros to convince gcc to let us put an enum in a bitfield, purely for the purpose of making it easier to read values in a debugger. I got rid of that code.
3. 32-bit x86 and ARM needed implementations of box/unboxDouble (which just redirect to store/loadDouble). I added them.

Combined with the previous three patches, this patch looks pretty good on try, except for SM(rust): https://treeherder.mozilla.org/#/jobs?repo=try&revision=279609dc122ba22a9a491647a223df2f9de902e5&selectedJob=240836842

The failure summary doesn't give much info, and I couldn't find anything useful in the logs, either.

As described in this comment re BigInt, changes to Value.h require corresponding changes to js/src/gdb/mozilla/jsval.py to keep the JS::Value prettyprinter working and js/rust/src/jsval.rs to keep the rust bindings working.

I've fixed the rust bindings. The pretty printer will take a bit more thought to figure out how to extract the double value.

I am currently working on splitting this all up into a cleaner patch stack for review.

Using the old NaN-boxing scheme, a zero-initialized value was a double, which was safe to trace. Under the new scheme, it is a null object pointer.

This patch manually initializes Value arrays to Undefined.

It took me a while to convince myself that this code was still okay for 0-tagged object Values. Adding a comment to make it clearer for future readers (and in the hope that a reviewer will notice my mistake if I am wrong.)

Depends on D29051

In the past, we have been somewhat sloppy when storing / loading double values, because a boxed double and an unboxed double had the same representation. This is no longer true with object-biased NaN-boxing. This patch goes through and cleans up those cases.

Depends on D29052

Similarly to Part 3: when we push a double value, we currently don't distinguish between pushing a boxed double and pushing an unboxed double. This has to change for object-biased NaN-boxing.

Depends on D29053

This patch is where the actual changes to our value representation happens. A few notes:

1. We did some weird macro tricks to work around a GCC bug with enums in bitfields. Those bitfields were only useful for poking at values in gdb, and the trick no longer worked with object-biased nanboxing, so I removed it. I also got rid of asDouble_, because it's no longer possible to read the double value right out of the enum without unboxing.

2. In the previous boxing scheme, there was a mechanical conversion between a JSValueType and a JSValueTag. That's no longer true, which is why the big conversion switches exist.

3. Waldo, you were included as a reviewer specifically to look at Value.h and make sure that our gross bit twiddling is just gross and not undefined.

Depends on D29054

We are changing the representation of values on 64-bit. Part 5 of this patch stack has more details on the changes.

Depends on D29055

This is a bit gross, but it passes all the tests that were not already failing.

Depends on D29056

(From: https://people.eecs.berkeley.edu/~krste/papers/EECS-2016-1.pdf)

Here are default NaN encodings for different platforms:

ISA Sign Significand QNaN Polarity

SPARC 0 11111111111111111111111 1
MIPS 0 01111111111111111111111 0
PA-RISC 0 01000000000000000000000 0
x86 1 10000000000000000000000 1
Alpha 1 10000000000000000000000 1
ARM 0 10000000000000000000000 1
RISC-V 0 10000000000000000000000 1

This seems like Part 5 is going to break on SPARC/MIPS. It seems like the general approach still works with an ADJ of 0x0008_0000_0000_0001 on those platforms. The mips simulator is a bit of a lie right now.

There should also be a few more static asserts, helper constexpr and a jsapi-test to validate this. We should also find someone with a physical sparc and mips to confirm at least a basic smoke test works. While they are unsupported platforms, it feels silly to break them with no notice in the next release to hit ESR. I think :jcristau might have access to some SPARC machines if I've remembering right.

Canceling the needinfo here, please re-request when the patch is ready for another round of fuzzing. Thanks!

Depends on D29870

Backed out for build bustages at MacroAssembler-inl.h

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

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251440145&repo=autoland&lineNumber=1013

Backout: https://hg.mozilla.org/integration/autoland/rev/1216e826db1110e05b73631342e0803b49d0fb7b

https://hg.mozilla.org/mozilla-central/rev/90769708bfc2
https://hg.mozilla.org/mozilla-central/rev/037012f898dd
https://hg.mozilla.org/mozilla-central/rev/098a1fc45a87
https://hg.mozilla.org/mozilla-central/rev/0636d29595ff
https://hg.mozilla.org/mozilla-central/rev/b82660f5a787
https://hg.mozilla.org/mozilla-central/rev/b5afafec0097
https://hg.mozilla.org/mozilla-central/rev/ae799ea48e2c
https://hg.mozilla.org/mozilla-central/rev/2ad263987c39