Implement sym operands for global_asm! #94468

Amanieu · 2022-03-01T02:00:19Z

Tracking issue: #93333

This PR is pretty much a complete rewrite of sym operand support for inline assembly so that the same implementation can be shared by asm! and global_asm!. The main changes are:

At the AST level, sym is represented as a special InlineAsmSym AST node containing a path instead of an Expr.
At the HIR level, sym is split into SymStatic and SymFn depending on whether the path resolves to a static during AST lowering (defaults to SynFn if get_early_res fails).
- SymFn is just an AnonConst. It runs through typeck and we just collect the resulting type at the end. An error is emitted if the type is not a FnDef.
- SymStatic directly holds a path and the DefId of the static that it is pointing to.
The representation at the MIR level is mostly unchanged. There is a minor change to THIR where SymFn is a constant instead of an expression.
At the codegen level we need to apply the target's symbol mangling to the result of tcx.symbol_name() depending on the target. This is done by calling the LLVM name mangler, which handles all of the details.
- On Mach-O, all symbols have a leading underscore.
- On x86 Windows, different mangling is used for cdecl, stdcall, fastcall and vectorcall.
- No mangling is needed on other platforms.

r? @nagisa
cc @eddyb

rust-highfive · 2022-03-01T02:00:22Z

Some changes occured to rustc_codegen_gcc

cc @antoyo

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

antoyo

The gcc part looks good to me.

Amanieu · 2022-03-01T16:23:01Z

The gcc part looks good to me.

Note that there is still an issue on the GCC side: symbols are not mangled correctly on Mach-O (leading underscore) and x86 Windows (cdecl/stdcall/fastcall/vectorcall mangling). This is why we need to call LLVM to get the full mangled symbol name on those platforms.

eddyb · 2022-03-02T08:01:54Z

compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp

+extern "C" void LLVMRustGetMangledName(LLVMValueRef V, RustStringRef Str) {
+  RawRustStringOstream OS(Str);
+  GlobalValue *GV = unwrap<GlobalValue>(V);
+  Mangler().getNameWithPrefix(OS, GV, true);
+}


I wonder if long-term we could implement this on the rustc side by getting the "mangling" flavor out of the datalayout string.

(Also, I wish both high-level mangling and this lower-level concept weren't both called "mangling" ugh)

Rustc already parses the datalayout string for type layouts.

Yeah, I was referring to the m:<mangling> field specifically. Though, looking at the docs:

x: Windows x86 COFF mangling: Private symbols get the usual prefix. Regular C symbols get a _ prefix. Functions with __stdcall, __fastcall, and __vectorcall have custom mangling that appends @N where N is the number of bytes used to pass parameters. C++ symbols starting with ? are not mangled in any way.

Yikes, that's a pretty hefty ask. Even if we generated it ourselves we'd probably still want to assert that we got it right by asking the backend what it would generate based on its own knowledge.

I wonder if long-term we could implement this on the rustc side by getting the "mangling" flavor out of the datalayout string.

LLVM seems to be moving in the direction of moving the responsibility for low-level mangling to the front-end: llvm/llvm-project#54090

eddyb

LGTM on the typeck/THIR side of things, nothing surprising there.

nagisa · 2022-03-05T18:54:10Z

src/test/ui/asm/type-check-1.rs

@@ -59,6 +61,11 @@ fn main() {
    }
 }

+unsafe fn generic<T>() {
+    asm!("{}", sym generic::<T>);


Does this actually work correctly when the generic_const_exprs is enabled? Could you add a test for that as well?

This fails with error: unconstrained generic constant and I'm not exactly sure how to go about fixing it.

To be fair, I feel like this should work, without any additional feature gates. I don't consider it a blocker for landing this PR, but I suspect it may require some significant changes to the approach (again) in order to make it work.

nagisa · 2022-03-05T18:57:27Z

src/test/ui/asm/type-check-1.stderr

+error[E0435]: attempt to use a non-constant value in a constant
+  --> $DIR/type-check-1.rs:47:24
+   |
+LL |         let x = 0;
+   |         ----- help: consider using `const` instead of `let`: `const x`
+...
+LL |         asm!("{}", sym x);
+   |                        ^ non-constant value


I would argue that the “sym operand must point to a function or static” kind of error would be more actionable here.

Especially because changing the let to a const won't help either way (I believe in any case, since FnDef types cannot be named)

I think this might be possible by using a custom rib kind in the resolver instead of using ConstantItemRibKind. I'll look into it.

I've reworked name resolution to generate a specific error for referring to a local in a sym operand.

compiler/rustc_builtin_macros/src/asm.rs

nagisa · 2022-03-05T19:45:21Z

compiler/rustc_passes/src/intrinsicck.rs

+                | hir::InlineAsmOperand::SplitInOut { .. } => {}
+                // No special checking is needed for these:
+                // - Typeck has checked that Const operands are integers.
+                // - AST lowering guarantees that SymStatic points to a static.


Does it guarantee that all sym STATICs become a SymStatic, though? It if does, that's also important to note (otherwise we'd start outputting errors in the branch below.)

Yes, the DefId for a static is resolved during AST lowering and kept in the operand.

compiler/rustc_passes/src/intrinsicck.rs

bors · 2022-03-06T10:07:13Z

☔ The latest upstream changes (presumably #90076) made this pull request unmergeable. Please resolve the merge conflicts.

joshtriplett · 2022-03-08T09:35:42Z

compiler/rustc_codegen_gcc/src/asm.rs

-                            // TODO(@Commeownist): This may not be sufficient for all kinds of statics.
-                            // Some statics may need the `@plt` suffix, like thread-local vars.
+                            // TODO(@Amanieu): Additional mangling is needed on
+                            // some targets to add a leading underscore (Mach-O).


Does this change imply correct handling of @PLT and similar? If so, is there a test to that effect?

We never add @ suffixes to symbol names, it's the user's responsibility to add any necessary suffixes.

Amanieu · 2022-04-04T01:50:34Z

Addressed all review feedback.

nagisa · 2022-04-10T12:15:08Z

@bors r+

🥳

bors · 2022-04-10T12:15:09Z

📌 Commit e0ed0f4 has been approved by nagisa

bors · 2022-04-15T18:48:03Z

⌛ Testing commit 2d90e592e668e7aa8fd72062a51e604d128c9e34 with merge 4f6221d74fef75b3704a708f9f6a97a6d5749072...

Dylan-DPC · 2022-04-15T18:51:33Z

@bors retry (yield to rollup)

bors · 2022-04-15T21:14:25Z

⌛ Testing commit 2d90e592e668e7aa8fd72062a51e604d128c9e34 with merge 276047d2ef1543eb466b420e5e82b72b912c9b5a...

bors · 2022-04-15T22:51:43Z

💔 Test failed - checks-actions

Amanieu · 2022-04-16T04:12:12Z

@bors r=nagisa

bors · 2022-04-16T04:12:13Z

📌 Commit bdba897 has been approved by nagisa

bors · 2022-04-16T04:46:03Z

⌛ Testing commit bdba897 with merge 080d545...

bors · 2022-04-16T06:55:22Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 080d545 to master...

rust-timer · 2022-04-16T08:09:15Z

Finished benchmarking commit (080d545): comparison url.

Summary:

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	0	0	1	0
mean²	N/A	N/A	N/A	-0.3%	N/A
max	N/A	N/A	N/A	-0.3%	N/A

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

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

Implement sym operands for global_asm! Tracking issue: rust-lang#93333 This PR is pretty much a complete rewrite of `sym` operand support for inline assembly so that the same implementation can be shared by `asm!` and `global_asm!`. The main changes are: - At the AST level, `sym` is represented as a special `InlineAsmSym` AST node containing a path instead of an `Expr`. - At the HIR level, `sym` is split into `SymStatic` and `SymFn` depending on whether the path resolves to a static during AST lowering (defaults to `SynFn` if `get_early_res` fails). - `SymFn` is just an `AnonConst`. It runs through typeck and we just collect the resulting type at the end. An error is emitted if the type is not a `FnDef`. - `SymStatic` directly holds a path and the `DefId` of the `static` that it is pointing to. - The representation at the MIR level is mostly unchanged. There is a minor change to THIR where `SymFn` is a constant instead of an expression. - At the codegen level we need to apply the target's symbol mangling to the result of `tcx.symbol_name()` depending on the target. This is done by calling the LLVM name mangler, which handles all of the details. - On Mach-O, all symbols have a leading underscore. - On x86 Windows, different mangling is used for cdecl, stdcall, fastcall and vectorcall. - No mangling is needed on other platforms. r? `@nagisa` cc `@eddyb`

rust-highfive assigned nagisa Mar 1, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2022

Amanieu mentioned this pull request Mar 1, 2022

Tracking Issue for asm_sym #93333

Closed

This comment has been minimized.

Sign in to view

Amanieu force-pushed the global_asm_sym branch from a2f7dd6 to ef56d5d Compare March 1, 2022 11:31

This comment has been minimized.

Sign in to view

Amanieu force-pushed the global_asm_sym branch from ef56d5d to a412fb2 Compare March 1, 2022 13:37

This comment has been minimized.

Sign in to view

antoyo approved these changes Mar 1, 2022

View reviewed changes

Amanieu force-pushed the global_asm_sym branch from a412fb2 to aa857d1 Compare March 1, 2022 16:21

eddyb reviewed Mar 2, 2022

View reviewed changes

eddyb approved these changes Mar 2, 2022

View reviewed changes

nagisa reviewed Mar 5, 2022

View reviewed changes

joshtriplett reviewed Mar 8, 2022

View reviewed changes

bstrie mentioned this pull request Mar 16, 2022

Compile with stable Rust enarx/enarx#1372

Open

12 tasks

pfmooney mentioned this pull request Mar 16, 2022

Keep an eye on asm_sym stabilization for MacOS consumers oxidecomputer/usdt#54

Closed

nagisa 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2022

Amanieu force-pushed the global_asm_sym branch from aa857d1 to e0ed0f4 Compare April 4, 2022 01:45

Amanieu removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 4, 2022

bstrie added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2022

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 Apr 10, 2022

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 Apr 15, 2022

This comment has been minimized.

Sign in to view

Amanieu force-pushed the global_asm_sym branch from 2d90e59 to a58ed0e Compare April 16, 2022 04:09

Update tests for sym support in global_asm!

bdba897

Amanieu force-pushed the global_asm_sym branch from a58ed0e to bdba897 Compare April 16, 2022 04:12

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 Apr 16, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 16, 2022

bors merged commit 080d545 into rust-lang:master Apr 16, 2022

rustbot added this to the 1.62.0 milestone Apr 16, 2022

This was referenced Apr 16, 2022

Move the extended lifetime resolution into typeck context #95563

Merged

Diagnose lifetime/label shadowing during late resolution #95781

Closed

Move various checks to typeck so them failing causes the typeck result to get tainted #96046

Merged

deltragon mentioned this pull request Apr 23, 2022

sym operands can not refer to generic parameters in recent nighty #96330

Closed

andylizi mentioned this pull request May 2, 2022

global_asm! sym operands emit functions without function body #96623

Closed

asquared31415 mentioned this pull request May 19, 2023

ICE attempting to indirectly call a function that takes a reference through asm! #111709

Open

Implement sym operands for global_asm! #94468

Implement sym operands for global_asm! #94468

Conversation

Amanieu commented Mar 1, 2022

rust-highfive commented Mar 1, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antoyo left a comment

Choose a reason for hiding this comment

Amanieu commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Mar 12, 2022 • edited Loading

Choose a reason for hiding this comment

nagisa Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Apr 4, 2022

nagisa commented Apr 10, 2022

bors commented Apr 10, 2022

bors commented Apr 15, 2022

Dylan-DPC commented Apr 15, 2022

This comment has been minimized.

bors commented Apr 15, 2022

bors commented Apr 15, 2022

This comment has been minimized.

Amanieu commented Apr 16, 2022

bors commented Apr 16, 2022

bors commented Apr 16, 2022

bors commented Apr 16, 2022

rust-timer commented Apr 16, 2022

Footnotes

nagisa Mar 12, 2022 •

edited

Loading

nagisa Mar 5, 2022 •

edited

Loading