1612057 - js engine assumptions incompatible with x86 5-level-paging

Status: UNCONFIRMED

(Core :: JavaScript Engine, task, P3)

Description

[lukas.bernhard]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

From the comments in https://searchfox.org/mozilla-central/source/memory/build/mozjemalloc.cpp#1500 I understand the js engine assumes the upper 17 bits in pointers are all 0. Ice Lake CPUs introduced 5-level-paging making 57 bits available for virtual addresses (needs to be enabled with linux kernel parameter CONFIG_X86_5LEVEL).

Actual results:

Lacking a Ice Lake CPU I don't have actual results to verify. But #589735 suggests the JS engine experiences unease if the assumption regarding the 17 leading 0 bits are violated.

Comment 1

[Jason Orendorff [:jorendorff]]

Marking P2 because nobody is going to take it immediately. These CPUs have been shipping since Q3 of 2019. We don't expect to see problems in desktop machines for some time, but it would not hurt to get out ahead of this. It'll take a lot of measurement.

Comment 2

[Nicolas B. Pierron [:nbp]]

If I recall correctly, we already have similar issue on other architecture, such as MIPS and ARM64.
In these cases we provided a desired set of pages to map, such that we could use the memory addresses which are well formatted to be used as pointers.

We might have to apply the same logic here.
Jon, would you happen to know where this code is located?

Comment 3

[Ted Campbell [:tcampbell]]

Luke pointed out to me that our pointers in Value come from the GC allocator only. This goes through the js::gc::Chunk allocator in [1]. As long as we continue to control the mappings there we should be fine.

See [2] for a more involved explanation of these concerns. That jemalloc comment is probably out-dated and maybe we should remove it.

[1] https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/js/src/gc/Allocator.cpp#824
[2] https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/js/src/gc/Memory.cpp#83-105

Comment 4

[Tooru Fujisawa [:arai]]

that doesn't apply to PrivateValue afaik.
https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/js/public/Value.h#788-804

it requires the 48-bit address space tho
https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/js/public/Value.h#189-193

Comment 5

[Ted Campbell [:tcampbell]]

Iain can probably comment on PrivateValue, but I believe we were being overly conservative there. We could allow up to 63-bits there as needed. Probably worth keeping track of.

Comment 6

[Jeff Walden [:Waldo]]

It will shortly be quite helpful for PrivateValue to encode numbers directly as uint64_t, which is probably the most desirable encoding format for ArrayBuffer byte lengths in a world where they are not limited to a parsimonious 2**31 - 1. If PrivateValue encoding changes, we're going to have to do a bit different (and possibly more laborious) hoop-jumping to deal with that.

Comment 7

[Ted Campbell [:tcampbell]]

PrivateValue will still always need to decode as DoubleValue so that GC tracing knows not to decode tags.

Comment 8

[Iain Ireland [:iain]]

Right now, as far as GC is concerned, private values are just doubles. This works, because every 48-bit userspace pointer is a non-NaN double. I was about to say that wasn't true for 57-bit pointers, but then I did the math and realized that it works out. For complicated reasons I think we may only be able to manage up to 62-bit pointers, but that should be good enough.

(Reasoning: the top bit is a sign bit. To be a NaN, the exponent (the next 11 bits) must be 1. If we limit addresses to 62 bits, then the highest exponent bit must be 0, so it won't be a NaN.)

If anybody ever tries shoving a 57-bit pointer in a private value, the assertion in Value.h will trigger and we can double-check my math.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Jeff Walden [:Waldo] from comment #6)

It will shortly be quite helpful for PrivateValue to encode numbers directly as uint64_t, which is probably the most desirable encoding format for ArrayBuffer byte lengths in a world where they are not limited to a parsimonious 2**31 - 1. If PrivateValue encoding changes, we're going to have to do a bit different (and possibly more laborious) hoop-jumping to deal with that.

I'm going to push back gently against that assertion, as I still don't understand why a Double would not be a good representation. For all practical purposes we no longer support systems without hardware FP, but we do support systems without 64-bit integer. Anyway this is bug 1392234.

(In reply to Iain Ireland [:iain] from comment #8)

For complicated reasons I think we may only be able to manage up to 62-bit pointers, but that should be good enough.

Possibly, but "that should be good enough" was the assertion about 48-bit pointers too, and that assertion blew up the moment we got a second 64-bit architecture to deal with. (It was a problem for SPARC before it became a problem for MIPS or arm64.)

If anybody ever tries shoving a 57-bit pointer in a private value, the assertion in Value.h will trigger and we can double-check my math.

I expect most distros will enable that flag once the feature's considered stable and since we're the default browser on many distros we'll start seeing problems quickly after that, unless we ensure that our beyond-48-bit VM workarounds are enabled for x64. Emanuel did a good job on those workarounds I think, but they are lightly tested and they are workarounds, and may have negative performance implications. See js/src/gc/Memory.cpp.

Comment 10

[Jon Coppeard (:jonco)]

(Ted answered this in comment 5.)