Emit error when trying to use assembler syntax directives in asm!
#82270
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (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. |
rust-highfive
added
the
S-waiting-on-review
|
I'm not sure if I like the exact phrasing of the error messages, so if anyone has any better ideas, please state them. |
jyn514
added
A-inline-assembly
|
@nagisa do you want to take this over? |
|
Yeah, can do. r? @nagisa |
src/test/ui/asm/inline-syntax.rs
Outdated
| @@ -0,0 +1,23 @@ | |||
| #![feature(asm, llvm_asm)] | |||
There was a problem hiding this comment.
This test needs to be conditional on target, or specify via compiler-flags what target to use. In later case, you will also need to make the test no_core – you can look at the other assembly tests (such as src/test/assembly/asm/x86-types.rs) to see how to set that up.
There was a problem hiding this comment.
The test should also check the behaviour when the target isn't x86_64, this can be done through revisions, you can see how those are used in the same x86-types.rs test as well.
|
Sorry for taking a while to get back to you, this is looking pretty nice now! Left some remaining nits, but the core logic looks pretty solid now. |
|
@bors r+ Thank you! |
|
📌 Commit 4b13b81 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 11 pull requests Successful merges: - rust-lang#82191 (Vec::dedup_by optimization) - rust-lang#82270 (Emit error when trying to use assembler syntax directives in `asm!`) - rust-lang#82434 (Add more links between hash and btree collections) - rust-lang#83080 (Make source-based code coverage compatible with MIR inlining) - rust-lang#83168 (Extend `proc_macro_back_compat` lint to `procedural-masquerade`) - rust-lang#83192 (ci/docker: Add SDK/NDK level 21 to android docker for 32bit platforms) - rust-lang#83204 (Simplify C compilation for Fortanix-SGX target) - rust-lang#83216 (Allow registering tool lints with `register_tool`) - rust-lang#83223 (Display error details when a `mmap` call fails) - rust-lang#83228 (Don't show HTML diff if tidy isn't installed for rustdoc tests) - rust-lang#83231 (Switch riscvgc-unknown-none-elf use lp64d ABI) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
| Some(InlineAsmArch::X86 | InlineAsmArch::X86_64) => rustc_ast::LlvmAsmDialect::Intel, | ||
| _ => rustc_ast::LlvmAsmDialect::Att, | ||
| } | ||
| } |
There was a problem hiding this comment.
The concept of an asm dialect doesn't exist outside of x86. In fact the .att_syntax and .intel_syntax directives are x86-specific and don't exist on other targets.
|
@asquared31415 |
…r=Dylan-DPC Fix test for rust-lang#82270 Fixes a test in rust-lang#82270 to require the arm llvm component
Rollup of 7 pull requests Successful merges: - rust-lang#82374 (Add license metadata for std dependencies) - rust-lang#82683 (Document panicking cases for integer division and remainder) - rust-lang#83272 (Clarify non-exact length in the Iterator::take documentation) - rust-lang#83338 (Fix test for rust-lang#82270) - rust-lang#83351 (post-drop-elab check-const: explain why we still check qualifs) - rust-lang#83367 (Improve error message for unassigned query provider) - rust-lang#83372 (SplitInclusive is public API) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Refactor rust-lang#82270 as lint instead of an error This PR fixes several issues with rust-lang#82270 which generated an error when `.intel_syntax` or `.att_syntax` was used in inline assembly: - It is now a warn-by-default lint instead of an error. - The lint only triggers on x86. `.intel_syntax` and `.att_syntax` are only valid on x86. - The lint no longer provides machine-applicable suggestions for two reasons: - These changes should not be made automatically since changes to assembly code can be very subtle. - The template string is not always just a string: it can contain macro invocation (`concat!`), raw strings, escape characters, etc. cc `@asquared31415`
Refactor rust-lang#82270 as lint instead of an error This PR fixes several issues with rust-lang#82270 which generated an error when `.intel_syntax` or `.att_syntax` was used in inline assembly: - It is now a warn-by-default lint instead of an error. - The lint only triggers on x86. `.intel_syntax` and `.att_syntax` are only valid on x86. - The lint no longer provides machine-applicable suggestions for two reasons: - These changes should not be made automatically since changes to assembly code can be very subtle. - The template string is not always just a string: it can contain macro invocation (`concat!`), raw strings, escape characters, etc. cc ``@asquared31415``
Rollup of 8 pull requests Successful merges: - rust-lang#83055 ([rustdoc] Don't document stripped items in JSON renderer.) - rust-lang#83437 (Refactor rust-lang#82270 as lint instead of an error) - rust-lang#83444 (Fix bootstrap tests on beta) - rust-lang#83456 (Add docs for Vec::from functions) - rust-lang#83463 (ExitStatusExt: Fix missing word in two docs messages) - rust-lang#83470 (Fix patch note about rust-lang#80653 not mentioning nested nor recursive) - rust-lang#83485 (Mark asm tests as requiring LLVM 10.0.1) - rust-lang#83486 (Don't ICE when using `#[global_alloc]` on a non-item statement) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The
.intel_syntaxand.att_syntaxassembler directives should not be used, in favor of not specifying a syntax for intel, and in favor of the explicitatt_syntaxoption using the inline assembly options.Closes #79869