Add support for specifying multiple clobber_abi in `asm!` #89316

asquared31415 · 2021-09-28T01:51:19Z

r? @Amanieu
cc #72016
@rustbot label: +A-inline-assembly +F-asm

asquared31415 · 2021-09-28T01:52:30Z

One concern I have is the increase of the size of ast::ItemKind, if that is a problem I think that it can be fixed with some boxed slices?

Amanieu · 2021-09-29T03:42:53Z

compiler/rustc_ast/src/ast.rs

@@ -2744,7 +2744,7 @@ pub enum ItemKind {
 }

 #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
-rustc_data_structures::static_assert_size!(ItemKind, 112);


Consider boxing the InlineAsm to avoid increasing the size of ItemKind.

Amanieu · 2021-09-29T03:45:05Z

compiler/rustc_ast_lowering/src/asm.rs

@@ -27,11 +28,13 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                .emit();
        }

-        let mut clobber_abi = None;
+        let mut clobber_abis = FxHashSet::default();


Use a HashMap keyed on the ABI, otherwise 2 identical ABIs with different spans will both be included.

compiler/rustc_ast_lowering/src/asm.rs

Amanieu · 2021-09-29T03:47:58Z

compiler/rustc_builtin_macros/src/asm.rs

@@ -210,7 +210,7 @@ fn parse_args<'a>(
                .span_labels(args.options_spans.clone(), "previous options")
                .span_label(span, "argument")
                .emit();
-        } else if let Some((_, abi_span)) = args.clobber_abi {
+        } else if let Some(&(_, abi_span)) = args.clobber_abis.last() {
            ecx.struct_span_err(span, "arguments are not allowed after clobber_abi")
                .span_label(abi_span, "clobber_abi")


I'd like this to point to all the clobber_abi spans, not just the last one.

Amanieu · 2021-09-29T03:48:13Z

compiler/rustc_builtin_macros/src/asm.rs

@@ -322,7 +322,7 @@ fn parse_args<'a>(
        // Bail out now since this is likely to confuse MIR
        return Err(err);
    }
-    if let Some((_, abi_span)) = args.clobber_abi {
+    if let Some(&(_, abi_span)) = args.clobber_abis.last() {
        if is_global_asm {
            let err =
                ecx.struct_span_err(abi_span, "`clobber_abi` cannot be used with `global_asm!`");


Amanieu · 2021-09-29T16:50:33Z

Can you add this to grammar in the asm documentation in the unstable book? Also in the reference-level explanation add a note saying that clobber_abi can be specified multiple times.

Amanieu · 2021-09-30T06:09:01Z

@bors r+

bors · 2021-09-30T06:09:03Z

📌 Commit b159d95 has been approved by Amanieu

… r=Amanieu Add support for specifying multiple clobber_abi in `asm!` r? `@Amanieu` cc rust-lang#72016 `@rustbot` label: +A-inline-assembly +F-asm

fee1-dead · 2021-10-01T17:08:04Z

@bors r-

Failed in #89420 (comment)

nbdd0121

I wonder if the syntax should be

clobber_abi("A"), clobber_abi("B")

or

clobber_abi("A", "B")

nbdd0121 · 2021-10-06T21:13:28Z

compiler/rustc_builtin_macros/src/asm.rs

@@ -322,10 +322,12 @@ fn parse_args<'a>(
        // Bail out now since this is likely to confuse MIR
        return Err(err);
    }
-    if let Some((_, abi_span)) = args.clobber_abi {
+    if args.clobber_abis.len() > 0 {


Suggested change

if args.clobber_abis.len() > 0 {

if !args.clobber_abis.is_empty() {

Amanieu · 2021-10-07T08:11:10Z

Hmm actually I slightly prefer clobber_abi("A", "B").

asquared31415 · 2021-10-08T00:27:01Z

I like that syntax better too

nbdd0121 · 2021-10-08T22:30:54Z

compiler/rustc_ast_lowering/src/asm.rs

+                            let mut abis = format!("`{}`", supported_abis[0]);
+                            for m in &supported_abis[1..] {
+                                let _ = write!(abis, ", `{}`", m);
+                            }


Suggested change

let mut abis = format!("`{}`", supported_abis[0]);

for m in &supported_abis[1..] {

let _ = write!(abis, ", `{}`", m);

}

let abis = supported_abis.join(",");

The code there adds ` around the abi name, which can't be replicated using just join like that.

nbdd0121 · 2021-10-08T22:32:29Z

compiler/rustc_ast_lowering/src/asm.rs

@@ -27,31 +28,36 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                .emit();
        }

-        let mut clobber_abi = None;
+        let mut clobber_abis = FxHashMap::default();


I think this shall be a Vec. If there are multiple occurrence of the same ABI then it should be error (and emitted during macro expansion), not silently merged.

nbdd0121 · 2021-10-08T22:33:36Z

compiler/rustc_ast_pretty/src/pprust/state.rs

-            args.push(AsmArg::ClobberAbi(abi));
+        if let Some((_, abis)) = &asm.clobber_abis {
+            for (abi, _) in abis {
+                args.push(AsmArg::ClobberAbi(*abi));


Wouldn't this still geenrate multiple clobber_abis?

nbdd0121 · 2021-10-08T22:37:34Z

compiler/rustc_builtin_macros/src/asm.rs

+                new_abis.push((str_lit.symbol_unescaped, str_lit.span));
+            }
+            Err(opt_lit) => {
+                // A closing paren is only allowed after a comma, so make sure we've seen an abi before, or else there's a missing abi argument


I think it's best to treat non-string-literal case and no ABI specified case separately (given that this is now a comma delimited list), so if the user writes clobber_abi() then we should say "at least an ABI name should be specified in clobber_abi" rather than just "expected string literal".

Amanieu · 2021-10-10T15:45:39Z

@bors r+

bors · 2021-10-10T15:45:40Z

📌 Commit 111423454809b548c7d8ad195d100046d99dcd98 has been approved by Amanieu

bors · 2021-10-10T22:01:47Z

⌛ Testing commit 111423454809b548c7d8ad195d100046d99dcd98 with merge f1b081eb34e3a1756703a5cd7884ae8132eb52ef...

Allow multiple clobber_abi in asm Update docs Fix aarch64 test Combine abis Emit duplicate ABI error, empty ABI list error multiple clobber_abi

Amanieu · 2021-11-10T10:16:57Z

@bors r+

bors · 2021-11-10T10:16:58Z

📌 Commit 3e799e4 has been approved by Amanieu

bors · 2021-11-10T12:32:45Z

⌛ Testing commit 3e799e4 with merge 4de694226be8edf717148c78451fac31fb9ff4f5...

bors · 2021-11-10T12:56:24Z

💔 Test failed - checks-actions

Amanieu · 2021-11-12T15:37:42Z

@bors r+

bors · 2021-11-12T15:37:43Z

📌 Commit e80f300 has been approved by Amanieu

bors · 2021-11-12T16:29:28Z

⌛ Testing commit e80f300 with merge 220ed09...

bors · 2021-11-12T19:27:58Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 220ed09 to master...

rust-timer · 2021-11-12T20:59:51Z

Finished benchmarking commit (220ed09): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rustbot added A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) labels Sep 28, 2021

rust-highfive assigned Amanieu Sep 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2021

asquared31415 force-pushed the multiple-clobber-abi branch from 77726da to db8af3f Compare September 28, 2021 02:17

Amanieu reviewed Sep 29, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2021

fee1-dead mentioned this pull request Oct 1, 2021

Rollup of 8 pull requests #89420

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 1, 2021

nbdd0121 reviewed Oct 6, 2021

View reviewed changes

asquared31415 force-pushed the multiple-clobber-abi branch from 59bae7f to 887caeb Compare October 8, 2021 21:08

nbdd0121 reviewed Oct 8, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

asquared31415 force-pushed the multiple-clobber-abi branch from 5625ff6 to 1114234 Compare October 9, 2021 02:45

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 10, 2021

This comment has been minimized.

Sign in to view

asquared31415 added 3 commits November 10, 2021 01:06

Add support for specifying multiple clobber_abi in asm!

b233d3b

Allow multiple clobber_abi in asm Update docs Fix aarch64 test Combine abis Emit duplicate ABI error, empty ABI list error multiple clobber_abi

Fix aarch tests

f3e42cb

add missing sym feature

3e799e4

asquared31415 force-pushed the multiple-clobber-abi branch from c77cd0a to 3e799e4 Compare November 10, 2021 06:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 10, 2021

fix aarch test error annotations

e80f300

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 12, 2021

bors merged commit 220ed09 into rust-lang:master Nov 12, 2021

rustbot added this to the 1.58.0 milestone Nov 12, 2021

asquared31415 deleted the multiple-clobber-abi branch November 18, 2021 17:58

asquared31415 mentioned this pull request Jun 15, 2023

ice: asm: unreachable #112635

Closed

	if args.clobber_abis.len() > 0 {
	if !args.clobber_abis.is_empty() {

Add support for specifying multiple clobber_abi in asm! #89316

Add support for specifying multiple clobber_abi in asm! #89316

Uh oh!

Conversation

asquared31415 commented Sep 28, 2021

Uh oh!

asquared31415 commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amanieu commented Sep 29, 2021

Uh oh!

Amanieu commented Sep 30, 2021

Uh oh!

bors commented Sep 30, 2021

Uh oh!

fee1-dead commented Oct 1, 2021

Uh oh!

nbdd0121 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amanieu commented Oct 7, 2021

Uh oh!

asquared31415 commented Oct 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbdd0121 Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Amanieu commented Oct 10, 2021

Uh oh!

bors commented Oct 10, 2021

Uh oh!

bors commented Oct 10, 2021

Uh oh!

This comment has been minimized.

Amanieu commented Nov 10, 2021

Uh oh!

bors commented Nov 10, 2021

Uh oh!

bors commented Nov 10, 2021

Uh oh!

This comment has been minimized.

bors commented Nov 10, 2021

Uh oh!

Amanieu commented Nov 12, 2021

Uh oh!

bors commented Nov 12, 2021

Uh oh!

bors commented Nov 12, 2021

Uh oh!

bors commented Nov 12, 2021

Uh oh!

rust-timer commented Nov 12, 2021

Uh oh!

Uh oh!

Add support for specifying multiple clobber_abi in `asm!` #89316

Add support for specifying multiple clobber_abi in `asm!` #89316

nbdd0121 Oct 8, 2021 •

edited

Loading