1587757 - Avoid using signal handling for unaligned FP accesses on ARM

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Lars T Hansen [:lth]]

Bug 1283121 added signal handlers for unaligned FP accesses on ARM, these handlers are triggered when the hardware is configured by the kernel not to accept such accesses. To do so, we included information about an Android-internal data structure whose copyright is in a gray area relative to the MPL; the solution has a fair amount of complexity; and we included syscall headers that are available on Linux and Android but not on all other Linux-derived systems, eg Raspbian and Gentoo, thus making life difficult for various distro maintainers.

We can almost certainly do something simpler than this: on systems with NEON, we can generate a VLD1 with byte alignment, and on systems without NEON (there are ever fewer of these) we can generate an unaligned integer load followed by a move-to-floating register. Since we don't support ARMv6 any more that should be enough.

We can then remove the complex signal handling code and the troublesome kernel data structure definition and the dependency on the syscall header.

Comment 1

[Lars T Hansen [:lth]]

This problem is starting to be an issue for high-profile distros and it would be best just to fix it.

Comment 2

[Lars T Hansen [:lth]]

Attachment: Bug 1587757 - Avoid signal handling for unaligned accesses. r?jseward

On ARM, the VLDR and VSTR instructions can't handle unaligned
accesses, instead they will generally trap with a SIGBUS. Some time
ago I added code to handle the trap and emulate the instructions in
the trap handler. This emulation is brittle in the sense that it
depends on information about kernel data structures, and that
information is not always available. So we need to handle this
differently, as follows.

This patch removes the SIGBUS handling and the load/store emulation,
and instead uses other instructions than VLDR/VSTR to handle FP loads
and stores.

On NEON-equipped systems (on almost all ARMv7 systems and on all ARMv8
systems), use the NEON VLD1 and VST1 instructions - they can deal with
unaligned data just fine.

On non-NEON-equipped systems, use a fallback path via integer load and
store, moving integer data to and from the FP register file.

Both paths depend on us seeing no unaligned traps, ie, either
SCTRL.A=0, or the kernel handling alignment traps when SCTRL.A=1. But
we've long since assumed that unaligned integer loads and stores will
Just Work under exactly those conditions, so this is going to have to
count as good enough. (And it's what V8 did, last I looked.)

As our assembler, simulator, and disassembler don't currently support
NEON much at all, I've had to add various support for VLD1/VST1, both
new variants and a drive-by fix to an existing VLD1 variant that
didn't handle wasm traps.

For code that does not use unaligned FP accesses, the performance of
the new NEON code should be the same as the old code, while the new
non-NEON code might be a hair slower (this is OK since almost all
systems have NEON). For code that does use unaligned FP accesses,
performance should be much better, as we avoid the signal handler.

Depends on D121832

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/baa2e3ee94bc
Avoid signal handling for unaligned accesses. r=jseward

Comment 4

[Marian-Vasile Laza]

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

Comment 5

[tt_1]

hey there, today I did test compile the fixes and can confirm they are working, and also did a runtime test with firefox-91.0esr on rpi2

there is a bit of a problem: spidermonkey shares its source with esr, so libmozjs.so is always built from esr.
the patches do apply pain free on top of firefox-91esr branch, but given the number of lines changed, I can't see distro maintainers are willing to take up the burden to fiddle with possible collision in future 91esr patch releases

so can you backport them to the firefox-91esr branch, please?

https://phabricator.services.mozilla.com/D121832
https://phabricator.services.mozilla.com/D121833

Comment 6

[Lars T Hansen [:lth]]

Yeah, that's the plan, see bug 1724475 comment 11. Just waiting for a resolution on the request to uplift to beta.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 9234841 [details]
Bug 1587757 - Avoid signal handling for unaligned accesses. r?jseward

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Some fairly knotty ARMv7-specific signal handling code in Firefox depends on kernel headers that are not available in all Linux distributions. Some distros that don't have the headers apply custom patches to work around the problem, but these patches are not always correct, nor are they desirable. Some of these distros are large and important (ask me for details). This patch and its two required companion patches (bug 1723930, bug 1724475) get rid of the signal handling and thus the build problems for these distros. They also simplify our code significantly.
• User impact if declined: Build problems for many Linux distros that build ESR91 will continue for the entire ESR91 window.
• Fix Landed on Version: FF92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Corner-case ARMv7 code which sees reasonable amounts of testing in the field. The alternative is to let the build problems persist for the entire ESR91 window.
• String or UUID changes made by this patch:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9234841 [details]
Bug 1587757 - Avoid signal handling for unaligned accesses. r?jseward

Approved for 91.1esr.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/234ffd840625