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

Transpile llvm_asm to asm #363

Closed
chrysn opened this issue Jan 7, 2022 · 7 comments · Fixed by #383
Closed

Transpile llvm_asm to asm #363

chrysn opened this issue Jan 7, 2022 · 7 comments · Fixed by #383

Comments

@chrysn
Copy link
Contributor

chrysn commented Jan 7, 2022

With the stabilization of the new asm language, the old llvm_asm is getting deprecated in Rust, creating the threat of eventual removal.

To build stable Rust code, we should aim to not just copy over the (LLVM / GCC style) assembly from C code, but try to change it to the newer (and stable) assembly language.

(#306 is different from this; that is just about emitting the old assembly with the new llvm_asm names).

I'm aware this is not a trivial request; it may be necessary to look for use cases that sponsor work on this.


Possible alternatives:

  • It may be possible to create extra assembly files for the blocks and then pull them in from the generated Rust code. It may also not.
  • Persuade Rust maintainers to keep llvm permanently deprecated -- discussion in Tracking Issue for LLVM-style inline assembly (llvm_asm) rust-lang/rust#70173 is not clear yet: deprecation happened and removal is announced, without any actual timeline for removal (or even a certainty that it will be removed)
  • Deprecate support for ASM in transpiled C.
chrysn added a commit to RIOT-OS-pkgmirror/riot-sys that referenced this issue Jan 12, 2022
It is tracked in immunant/c2rust#363 -- having
it around hiding other warnings in the flood doesn't do any good either.
@Amanieu
Copy link

Amanieu commented Jan 13, 2022

llvm_asm! is being removed in rust-lang/rust#92816.

@chrysn
Copy link
Contributor Author

chrysn commented Jan 13, 2022 via email

@Amanieu
Copy link

Amanieu commented Jan 13, 2022

From what I can tell the assembly currently produced by c2rust doesn't even work on the latest nightly (or in fact any nightly in the last 2 years) since it's using the old LLVM assembly syntax with asm!.

@chrysn
Copy link
Contributor Author

chrysn commented Jan 13, 2022

It's working alright for my use case in the RIOT project (with some fixes that are all in open PRs -- one to rename it to llvm_asm, one to fix some lifetime issues IIRC).


I've been doing some tests on viable alternatives, and from the looks of it, little of the ASM that's part of the everyday code gets actually used, leaving (in the one example I've had a look at) only one actually used item. My current workaround is manually doing the changes on selected ASM statements, about like this -- with an unavailable symbol to squeak if any non-transpiled assembly does make it past dead code elimination into the linker.

extern "C" { fn asm_is_not_supported_any_more(); }

macro_rules! llvm_asm {
    ("MRS $0, ipsr" : "=r" ($result:ident) : : : "volatile") => {
        core::arch::asm!("MRS {}, ipsr", out(reg) $result);
    };
    ($($x:tt)*) => {{
        asm_is_not_supported_any_more();
        unreachable!()
    }};
}

(With no guarantees for equivalence of the two lines).

@fw-immunant
Copy link
Contributor

llvm_asm was recently removed, which makes this a bit more urgent as a requirement for supporting current rustc.

@fw-immunant
Copy link
Contributor

I've done a (fairly unpolished, and assuming x86_64 in a few places) pass at this in the syn PR, because it's a necessary part of bringing us up to speed with the rest of the Rust ecosystem. Test coverage is still lacking, so any feedback would be particularly appreciated.

@fw-immunant
Copy link
Contributor

#374 has merged, but I'll leave this bug open until we have broader test coverage to better validate the asm translation. Currently there's some minimal asm for a bswap implementation that gets dragged in from system headers, a few places doing cpuid, and a few mathy bits from python in the testsuite, but we would do well to have targeted test cases for inline asm in the c2rust repo itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants