panic_unwind: Use global_asm! for IMGREL relocations

[Darksonn]

View all comments

Rust consteval cannot construct 32-bit values that contain the offset between two symbols, but this is required for panic_unwind on some architectures because unwinding on Windows SEH uses image-base-relative pointers for the panic information (which makes them fit in 32-bit rather than 64-bit).

Currently, this is worked around by initializing these globals on panic with atomic stores. This works, but is undefined behavior as the panic runtime reads the values with non-atomic loads, which leads to a data race.

Instead, utilize global_asm! to create the globals with @IMGREL relocations so that the linker constructs the relative pointers for us at link or load time.

For context: #t-compiler > relative pointers in windows SEH panic_unwind @ 💬

AI assistance was involved with writing the assembly code.

[rustbot]

r? @LawnGnome

rustbot has assigned @LawnGnome.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 7 candidates

[Fulgen301]

(Nitpick: I'd clarify that they're offsets from the image base and not __ImageBase, even if that's the exact same address.)

View changes since the review

[LawnGnome]

Re-rolling, as my asm is absolutely not good enough for this.

@rustbot reroll

[Mark-Simulacrum]

Definitely not familiar with the Windows aspects of this... but happy to approve given Bjorn has taken a look with a response to the comment (either telling me I'm wrong or changing things :)

View changes since this review

[Mark-Simulacrum]

Isn't the range of this offset too small?

Is the program label whose address is to be calculated. It is an offset from the address of this instruction, in the range ±1MB.

(https://support.arm.com/documentation/dui0802/b/A64-General-Instructions/ADR)

I would expect we need to support programs where the instructions and rdata are farther apart. Should we be using adrl here?

(Not sure if lea has the same problem).

[bjorn3]

Using adrl makes sense to me. For lea, it can be encoded with a 16bit, 32bit or 64bit offset. I don't know which one LLVM picks. Probably either 32bit or 64bit.

[Mark-Simulacrum]

I'm hoping this is the cause of the exit code: 0xc0000005, STATUS_ACCESS_VIOLATION in the try job, but once fixed we should re-run that. If it is the cause, I'm sad that LLVM / rustc didn't complain about the relocation overflowing, but maybe it's not that surprising.

[Darksonn]

It looks like using wider offsets fixed the issue.

[Mark-Simulacrum]

@bors try jobs=msvc

[rust-bors]

💔 Test for ee5da48 failed: CI. Failed jobs:

• try - aarch64-msvc-1 (web logs, enhanced plaintext logs)
• try - aarch64-msvc-2 (web logs, enhanced plaintext logs)

[Darksonn]

@bors try jobs=msvc

[rust-bors]

💔 Test for aa22e3c failed: CI. Failed jobs:

• try - aarch64-msvc-1 (web logs, enhanced plaintext logs)
• try - aarch64-msvc-2 (web logs, enhanced plaintext logs)
• try - dist-aarch64-msvc (web logs, enhanced plaintext logs)

[Darksonn]

@bors try jobs=msvc

[rust-bors]

☀️ Try build successful (CI)
Build commit: 976338d (976338db6bad68e99abbdc7370002f113822c140)
Base parent: 7088e4b (7088e4b63a9516ebfbfe2ab2d999cf01a528ac14)