wasmtime / PR #6384 aarch64: Fix Ldr19 relocations being ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6384 aarch64: Fix Ldr19 relocations being ...

Wasmtime GitHub notifications bot (May 16 2023 at 01:29):

alexcrichton opened PR #6384 from alexcrichton:fix-ldr19-out-of-bounds to bytecodealliance:main:

This commit is a suite of fixes and support to fix a bug on the AArch64 backend, and others. Before this commit constants were unconditionally placed at the end of a function which means that if the function was too large then references from the beginning of the function may be too far away. The fix here is a bit subtle but is generally to emit all previously referenced constants when an island is emitted, resetting constants to get emitted on the next island. This means that constants may be emitted multiple times throughout function emission, which solves both a reference being too far in front of you and behind you.

I've done testing of this with a new Cranelift setting that automatically inserts padding between basic blocks. This setting is additionally hooked up to the fuzzer to help uncover issues like this on OSS-Fuzz.

Wasmtime GitHub notifications bot (May 16 2023 at 01:29):

alexcrichton requested jameysharp for a review on PR #6384.

Wasmtime GitHub notifications bot (May 16 2023 at 01:29):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6384.

Wasmtime GitHub notifications bot (May 16 2023 at 01:29):

alexcrichton requested wasmtime-core-reviewers for a review on PR #6384.

Wasmtime GitHub notifications bot (May 16 2023 at 01:43):

cfallin assigned PR #6384 to cfallin.

Wasmtime GitHub notifications bot (May 16 2023 at 02:01):

cfallin submitted PR review:

This looks generally great -- thanks for tackling this! A few comments below mostly about cosmetics/docs but otherwise LGTM.

Wasmtime GitHub notifications bot (May 16 2023 at 02:01):

cfallin submitted PR review:

This looks generally great -- thanks for tackling this! A few comments below mostly about cosmetics/docs but otherwise LGTM.

Wasmtime GitHub notifications bot (May 16 2023 at 02:01):

cfallin created PR review comment:

Not too important here but FWIW a pattern I've used before for this kind of setting is "log2 - 1", which allows the zero baseline and still gives all powers of two.

Wasmtime GitHub notifications bot (May 16 2023 at 02:01):

cfallin created PR review comment:

A little more precise wording than "change over time" maybe: "this function may return a different label for the same constant at different points in time. The label is valid to use only from the current location; the MachBuffer takes care to emit the same constant multiple times if needed so the constant is always in range."

Wasmtime GitHub notifications bot (May 16 2023 at 02:01):

cfallin created PR review comment:

FWIW, I wonder if it isn't better to directly plumb the settings through to where VCode is created (or VCodeBuilder::new or similar); or else to emit() directly; the EmitInfo is part of the "assembler layer", and we want to keep this as loosely coupled as possible I think. (Not least for Winch's sake, but also just to avoid unnecessary entangling.)

Wasmtime GitHub notifications bot (May 16 2023 at 02:01):

cfallin created PR review comment:

The "undefined" bit I think can be left out here (or pushed into the body if you want to keep it): it's not supposed to be visible to the user of MachBuffer whether a label is defined yet or not, and we don't want to make it part of the contract.