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

i128 abi incompatibility with cg_llvm #1449

Closed
DrewRidley opened this issue Jan 11, 2024 · 13 comments · Fixed by bytecodealliance/wasmtime#7770
Closed

i128 abi incompatibility with cg_llvm #1449

DrewRidley opened this issue Jan 11, 2024 · 13 comments · Fixed by bytecodealliance/wasmtime#7770
Labels
A-abi Area: ABI handling C-bug Category: This is a bug.

Comments

@DrewRidley
Copy link

DrewRidley commented Jan 11, 2024

Hi all,

I was informed a little over a week ago that it should be possible to mix LLVM and cranelift in the same rust target. I went about writing a game in bevy, and using opt = 3 with LLVM for my dependencies. I used the following toml file to achieve this:

[profile.dev.package."*"]
codegen-backend = "llvm"

[profile.dev]
codegen-backend = "cranelift"

I then added bevy as a dependency, and ran the simple example provided. All was well and the examples listed on their website run as expected. However, as soon as a Plugin is written in a cranelift context, and added to the LLVM emitted bevy code, a segmentation fault occurs. I suspect it might have to do with calling conventions, but my knowledge about LLVM and Cranelift is limited so I won't be much help here.

Here is a minimal example that should illustrate the problem:

use bevy::prelude::*;

pub struct MyCraneliftPlugin;

impl Plugin for MyCraneliftPlugin {
    fn build(&self, app: &mut App) {
       app.add_systems(Update, || { debug!("Hello world!") });
    }
}

   fn main() {
    App::new()
        .add_plugins(MinimalPlugins)
        .add_plugins(MyCraneliftPlugin)
        .run();
}

Compiling the following exclusively with LLVM or exclusively with Cranelift works as expected. Mixing the backends with the config provided earlier causes a segfault. I tried to investigate where exactly the segmentation fault occurs with lldb but was largely unsuccessful as it was not clear which code emitted was from cranelift and which was llvm.

Any help would be much appreciated, and I apologize I could not be more help here.

@bjorn3
Copy link
Member

bjorn3 commented Jan 11, 2024

I can reproduce the crash with the following Cargo.toml:

[package]
name = "crash_repro"
version = "0.1.0"
edition = "2021"

[dependencies]
bevy = "0.12.1"

[profile.dev.package."*"]
codegen-backend = "llvm"
opt-level = 3

[profile.dev]
codegen-backend = "cranelift"

@bjorn3 bjorn3 added the C-bug Category: This is a bug. label Jan 11, 2024
@bjorn3
Copy link
Member

bjorn3 commented Jan 11, 2024

Found the issue: LLVM allows returning values in up to 3 registers, while Cranelift uses an implicit return value argument after the second register even when enable_llvm_abi_extensions is used: https://github.com/bytecodealliance/wasmtime/blob/536cf88ce9864cdddfe52bf625c81b06e8eb1d43/cranelift/codegen/src/isa/x64/abi.rs#L1058-L1059 This should be easy to fix.

@bjorn3
Copy link
Member

bjorn3 commented Jan 11, 2024

bytecodealliance/wasmtime#7770 should fix this.

@bjorn3
Copy link
Member

bjorn3 commented Jan 11, 2024

By the way thanks for providing the repro! It was really useful for figuring out the problem.

@bjorn3
Copy link
Member

bjorn3 commented Jan 11, 2024

The Cranelift PR has been accepted and backported to the branch that will be released in a little longer than a week. Once it has been released I will update cg_clif.

@bjorn3
Copy link
Member

bjorn3 commented Jan 26, 2024

Updated Cranelift in dc7ed16. Got a couple of other things I want to do first before syncing to the rust repo to get it on nightly. Will let you know when this is in a new nightly.

@bjorn3
Copy link
Member

bjorn3 commented Jan 28, 2024

Should be fixed on the latest nightly.

@bjorn3
Copy link
Member

bjorn3 commented Mar 6, 2024

Seems like it isn't fixed after all.

@bjorn3
Copy link
Member

bjorn3 commented Mar 31, 2024

Tried it again. Can't reproduce this crash anymore. abi-cafe does still show abi incompatibilities with cg_llvm for the i128 handling though, possibly because of https://blog.rust-lang.org/2024/03/30/i128-layout-update.html

@bjorn3 bjorn3 added the A-abi Area: ABI handling label Mar 31, 2024
@bjorn3 bjorn3 changed the title LLVM Dependency Segfault i128 abi incompatibility with cg_llvm Mar 31, 2024
@bjorn3
Copy link
Member

bjorn3 commented Mar 31, 2024

Will be fixed by bytecodealliance/wasmtime#8270 on x86_64-unknown-linux-gnu at least.

@bjorn3
Copy link
Member

bjorn3 commented Apr 1, 2024

With a cg_clif patched to use current Cranelift main (which contains bytecodealliance/wasmtime#8270) https://github.com/rust-lang/rustc_codegen_cranelift/actions/runs/8508177743 shows that abi-cafe now passes on x86_64-unknown-linux-gnu with the i128 test expectations updated to indicate that they are no longer broken. On x86_64-apple-darwin interoperability with cg_llvm works again too, but the clang used on GHA doesn't yet have the ABI fixes, so cg_clif <-> C is still broken for i128.

@bjorn3
Copy link
Member

bjorn3 commented Apr 22, 2024

Updated cranelift in 6ec27fe. Will sync it back to the rust repo tomorrow.

@bjorn3
Copy link
Member

bjorn3 commented May 3, 2024

This has been fixed on nightly now.

@bjorn3 bjorn3 closed this as completed May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-abi Area: ABI handling C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants