1765346 - Consider removing alignas(8) from JS::Value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

We can't pass JS::Value by value as function argument due to its alignas(8). It's not clear to me if/why we still need this. Value contains just a uint64_t asBits_ field nowadays.

If I remove the alignas, things seem to work fine. We need some JIT codegen changes for 32-bit platforms where Map/Set entries are a bit smaller now, but that's easy to fix.

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

My understanding of this alignas is the requirement that JS::Value hold double value which are read/written as double and some architecture cause a SIGBUS when the memory is not properly aligned. (ARM?)

Comment 2

[Jan de Mooij [:jandem]]

Good point, but alignof(Value) would still be 8 on 32-bit ARM due to its uint64_t member.

There could be an issue with the ARM simulator because it has LDRD/STRD alignment assertions and compiles for x86-32 where the alignment is 4. It does pass all jit-tests for me though.

Comment 3

[Lars T Hansen [:lth]]

Unaligned FP loads are dicey on some ARM systems depending on the code the C++ compiler generates. Generally these days it seems that the kernel will patch up for you if you have an unaligned integer access, if the CPU traps, but for FP accesses you must generate the correct unaligned-friendly instruction to avoid the trap.

Comment 4

[Jan de Mooij [:jandem]]

NI myself to try changing this and if nothing comes up after a few months we can start changing const Value& arguments to Value.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1765346 part 1 - Remove alignas(8) and MOZ_NON_PARAM from JS::Value. r?iain!

Some Clang versions had bugs when passing types that use alignas as argument
to a function, so our static analysis currently doesn't allow this. This meant
that JS::Value had to be passed as const-ref (const Value&) instead.

JS::Value has a uint64_t field and this ensures the alignment will be 8 on most
platforms also without the alignas. The main exception is 32-bit x86 where Value
can now be 4-byte aligned in C++ code.

This patch fixes two places to avoid static_assert failures in the JIT backend on x86.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1765346 part 2 - Pass JS::Value by value in a few places. r?iain!

This just converts a few places for now.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9a25662d7155
https://hg.mozilla.org/integration/autoland/rev/8f7f6165c166
part 1 - Remove alignas(8) and MOZ_NON_PARAM from JS::Value. r=iain
https://github.com/mozilla-firefox/firefox/commit/5cd3a0e47dfe
https://hg.mozilla.org/integration/autoland/rev/25b300930803
part 2 - Pass JS::Value by value in a few places. r=iain

Comment 8

[amarc]

https://hg.mozilla.org/mozilla-central/rev/8f7f6165c166
https://hg.mozilla.org/mozilla-central/rev/25b300930803

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1765346 part 3 - Fix a comment. r?iain!

Also delete the part about JSVAL_* operations because these were removed a long
time ago (bug 952650 for example).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0bb4c75953a9
https://hg.mozilla.org/integration/autoland/rev/b60cdef109c8
part 3 - Fix a comment. r=iain

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/b60cdef109c8

Comment 12

[BugBot [:suhaib / :marco]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)