Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cranelift: Remove srcloc from emit state on all targets #8122

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

jameysharp
Copy link
Contributor

In #2426, @cfallin wrote:

[…] don't emit trap info unless an op can trap.

This end result was previously enacted by carrying a SourceLoc on
every load/store, which was somewhat cumbersome, and only indirectly
encoded metadata about a memory reference (can it trap) by its
presence or absence.

That PR changed both backends that existed at the time to check both the source location and the memory flags to determine whether a memory access could trap.

Then in #2685, @cfallin wrote:

Finally, while working out why trap records were not appearing, I had
noticed that isa::x64::emit_std_enc_mem() was only emitting heap-OOB
trap metadata for loads/stores when it had a srcloc. This PR ensures
that the metadata is emitted even when the srcloc is empty.

However that PR did not apply the same change to other backends. Since then, the pattern from #2426 has been copied to new backends.

I believe checking the source location has been unnecessary since #2426 and is now just a source of confusion at best, and possibly bugs at worst. So this PR makes all targets match the behavior of the x64 backend.

In addition, this pattern was the only reason why source locations were provided to any backend's emit state, so I'm removing that entirely. The cur_srcloc field has been unused on x64 since #2685.

This change is mostly straightforward, but there are two questionable changes in the riscv64 backend:

  • The riscv64 backend had one use of this pattern for a BadConversionToInteger trap. All other uses on all backends were for HeapOutOfBounds traps. I suspect that was a copy-paste bug so I've removed it just like all the others.

  • The riscv64 Inst::Atomic does not have a MemFlags field, so this means the HeapOutOfBounds trap metadata is added unconditionally for such instructions.

In bytecodealliance#2426, @cfallin wrote:

> […] don't emit trap info unless an op can trap.
>
> This end result was previously enacted by carrying a SourceLoc on
> every load/store, which was somewhat cumbersome, and only indirectly
> encoded metadata about a memory reference (can it trap) by its
> presence or absence.

That PR changed both backends that existed at the time to check both the
source location and the memory flags to determine whether a memory
access could trap.

Then in bytecodealliance#2685, @cfallin wrote:

> Finally, while working out why trap records were not appearing, I had
> noticed that isa::x64::emit_std_enc_mem() was only emitting heap-OOB
> trap metadata for loads/stores when it had a srcloc. This PR ensures
> that the metadata is emitted even when the srcloc is empty.

However that PR did not apply the same change to other backends. Since
then, the pattern from bytecodealliance#2426 has been copied to new backends.

I believe checking the source location has been unnecessary since bytecodealliance#2426
and is now just a source of confusion at best, and possibly bugs at
worst. So this PR makes all targets match the behavior of the x64
backend.

In addition, this pattern was the only reason why source locations were
provided to any backend's emit state, so I'm removing that entirely.
The `cur_srcloc` field has been unused on x64 since bytecodealliance#2685.

This change is mostly straightforward, but there are two questionable
changes in the riscv64 backend:

- The riscv64 backend had one use of this pattern for a
  BadConversionToInteger trap. All other uses on all backends were for
  HeapOutOfBounds traps. I suspect that was a copy-paste bug so I've
  removed it just like all the others.

- The riscv64 `Inst::Atomic` does not have a MemFlags field, so this
  means the HeapOutOfBounds trap metadata is added unconditionally for
  such instructions.
@jameysharp jameysharp requested a review from cfallin March 13, 2024 20:04
@jameysharp jameysharp requested a review from a team as a code owner March 13, 2024 20:04
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- thanks for finding this cleanup!

FWIW it should always be safe to emit extra trap records -- the "record exists and wasn't supposed to case" would matter only in that it could mask a separate bug, wherein the lowering generated a trap it wasn't supposed to, and turn it into a Wasm-level trap rather than a process crash. And that itself only if we use the instruction for internal logic separately from accesses that correspond to Wasm-level operators. So I think this is fine re: riscv64's Atomic MachInst.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels Mar 13, 2024
@jameysharp jameysharp added this pull request to the merge queue Mar 13, 2024
Merged via the queue into bytecodealliance:main with commit c423a69 Mar 13, 2024
24 checks passed
@jameysharp jameysharp deleted the no-trap-srcloc branch March 13, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants