Implement global_asm!() (RFC 1548) #40702
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I will rebase this evening. |
|
If an LLVM API is not provided by the C API, you can add a C++ wrapper in rustllvm and the corresponding Rust types in librustc_llvm. Look at the rest of the file to see the conventions that are used (to_rust, from_rust, etc.) |
|
Thank you @mattico! @steveklabnik I updated the unstable-book, so you might want to look it over. |
| my_asm_func: | ||
| ret | ||
| __other_cancel: | ||
| jmp __cancel |
There was a problem hiding this comment.
This would need to be mangled, wouldn't it?
There was a problem hiding this comment.
ah, yes, we need a #[no_mangle]. Thanks!
|
We have a feature-gate test and a couple codegen tests! We also have a new unstable-book page. My initial problems/questions have been resolved. |
mrhota
changed the title
| ```rust,ignore | ||
| # #![feature(global_asm)] | ||
| # you also need relevant target_arch cfgs | ||
| global_asm!(include_str("something_neato.s")); |
There was a problem hiding this comment.
This should be include_str! (missing !).
|
r? @pnkfelix |
|
@Amanieu we have a new test using |
|
@mrhota Yes, that's what I had in mind. |
|
@eddyb @nagisa @michaelwoerister any thoughts here? not sure who should review |
There was a problem hiding this comment.
Seems good to me overall.
I’d like, however, more tests. Namely to ensure stuff like these compile. Compilation here is the important distinction, as codegen tests do not actually go through the assembler, and only generate LLVM-IR, which does not insure the actual codegen works.
// cfg-gating works properly
#[cfg(target_arch="x86")] global_asm! { ... }
#[cfg(target_arch="x86_64")] global_asm! { ... }
// codegen tests for all borderline T-1 architectures
global_asm!("foo blt foo") // arm
global_asm!("foo: j foo") // mips
global_asm!("foo: b foo") // ppc
src/test/codegen/global_asm.rs
Outdated
| // except according to those terms. | ||
|
|
||
| // ignore-arm | ||
| // ignore-aarch64 |
There was a problem hiding this comment.
Since assembly is for x86, this list should be significantly longer.
Namely we also support: MIPS, PowerPC, SystemZ, wasm, asmjs, nacl, nvptx, avr, msp430 and probably more stuff I forgot to mention.
There was a problem hiding this comment.
A good approach would be to git grep '// ignore-' src/tests and add every single suffix that's seems unrelated to x86.
| // except according to those terms. | ||
|
|
||
| // ignore-arm | ||
| // ignore-aarch64 |
There was a problem hiding this comment.
Since assembly is for x86, this list should be significantly longer.
There was a problem hiding this comment.
I copied this from another codegen test, I think. I'll fill out the list.
src/libsyntax/ast.rs
Outdated
| #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)] | ||
| pub struct GlobalAsm { | ||
| pub asm: Symbol, | ||
| pub asm_str_style: StrStyle, |
|
@nagisa I might be missing something, but what's the point of testing full-compile in addition to codegen? The real meat of this feature enables insertion of If we test full-compile, then we're just testing that LLVM does what it says it'll do with global_asm!("put on your time travel trousers!");should in fact go all the way through codegen, and should produce which will obviously fail to assemble, but that's not our problem. The assembly template provided to But, again, the codegen tests ensure |
This is irrelevant from the perspective of the end user – they use rustc, not LLVM. You can assign blame to some dependency, but the fact of dependencies not working propagates to the end products. Now, I don’t feel strongly about this, since this is will be unstable for a foreseeable future, but such tests will have to materialise before this gets stabilised. Might as well materialise them right now, then, right? EDIT: It might as well point out the fact of us having LLVM misconfigured in a way that it is unable to work with the module level assembly :) |
|
@nagisa I'm at a loss; I have no idea why the builder keeps failing codegen tests since I added more |
|
r? @nikomatsakis you seemed to be somewhat interested in how this feature turns out |
|
☔ The latest upstream changes (presumably #40899) made this pull request unmergeable. Please resolve the merge conflicts. |
I’ve got no idea why it fails either. I’ll make a mental note to try and build this locally, but am likely to forget. |
|
@nagisa thanks. I ran the failing docker container's build on my mac and it passed. I ran the tests on my mac, too, and they pass. |
|
I don't see a box to check. Anyways, I'm happy with the changes. |
|
💔 Test failed - status-appveyor |
|
@bors retry
|
|
⌛ Testing commit 63a0747 with merge a2e97b2... |
|
💔 Test failed - status-appveyor |
|
same SSL error on Windows |
|
@bors: retry
|
Implement global_asm!() (RFC 1548) This is a first attempt. ~~One (potential) problem I haven't solved is how to handle multiple usages of `global_asm!` in a module/crate. It looks like `LLVMSetModuleInlineAsm` overwrites module asm, and `LLVMAppendModuleInlineAsm` is not provided in LLVM C headers 😦~~ I can provide more detail as needed, but honestly, there's not a lot going on here. r? @eddyb CC @Amanieu @jackpot51 Tracking issue: #35119
|
💔 Test failed - status-appveyor |
|
That appveyor failure looks legit. |
@alexcrichton @nagisa any pointers? I don't have access to a Also, note that |
|
The failure here is:
That's probably because of the way symbols are named across systems. I believe on some platforms (like OSX and Windows) you need a leading underscore (e.g. |
|
@alexcrichton that seems to be it. Thanks! |
Mark-Simulacrum
added
the
S-waiting-on-author
|
@alexcrichton ready again... I declared both |
|
@bors r=nagisa |
|
📌 Commit a35c4e3 has been approved by |
Implement global_asm!() (RFC 1548) This is a first attempt. ~~One (potential) problem I haven't solved is how to handle multiple usages of `global_asm!` in a module/crate. It looks like `LLVMSetModuleInlineAsm` overwrites module asm, and `LLVMAppendModuleInlineAsm` is not provided in LLVM C headers 😦~~ I can provide more detail as needed, but honestly, there's not a lot going on here. r? @eddyb CC @Amanieu @jackpot51 Tracking issue: rust-lang#35119
|
⌛ Testing commit a35c4e3 with merge ea2c4f5... |
|
@bors retry reprioritizing |
This is a first attempt.
One (potential) problem I haven't solved is how to handle multiple usages ofglobal_asm!in a module/crate. It looks likeLLVMSetModuleInlineAsmoverwrites module asm, andLLVMAppendModuleInlineAsmis not provided in LLVM C headers 😦I can provide more detail as needed, but honestly, there's not a lot going on here.
r? @eddyb
CC @Amanieu @jackpot51
Tracking issue: #35119