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

Tracking Issue for asm_sym #93333

Closed
Amanieu opened this issue Jan 26, 2022 · 47 comments
Closed

Tracking Issue for asm_sym #93333

Amanieu opened this issue Jan 26, 2022 · 47 comments
Labels
A-inline-assembly Area: inline asm!(..) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-asm `#![feature(asm)]` (not `llvm_asm`) finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Amanieu
Copy link
Member

Amanieu commented Jan 26, 2022

The feature gate for the issue is #![feature(asm_sym)].

Summary

This feature adds a sym <path> operand type to asm! and global_asm!.

  • <path> must refer to a fn or static.
  • A mangled symbol name referring to the item is substituted into the asm template string.
  • The substituted string does not include any modifiers (e.g. GOT, PLT, relocations, etc).
  • <path> is allowed to point to a #[thread_local] static, in which case the asm code can combine the symbol with relocations (e.g. @plt, @TPOFF) to read from thread-local data.

Status

Blocked on support for sym in global_asm!.

@Amanieu Amanieu added A-inline-assembly Area: inline asm!(..) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-asm `#![feature(asm)]` (not `llvm_asm`) labels Jan 26, 2022
@bstrie
Copy link
Contributor

bstrie commented Jan 27, 2022

Is this note from #72016 still accurate?

The way sym is handled by the compiler (in HIR & typeck) is likely to change in the future to support it in global_asm. Currently it's a Path that's evaluated in the typeck context of the function containing the asm! but that doesn't work with global_asm! which exists outside of a function body.

@Amanieu
Copy link
Member Author

Amanieu commented Jan 27, 2022

Yes it is.

@dylanede
Copy link
Contributor

Is there any chance of splitting stabilisation of this so that sym in asm! can be stabilised first, without waiting for global_asm! support?

@bstrie
Copy link
Contributor

bstrie commented Feb 17, 2022

@dylanede Interesting, that seems like a reasonable idea. global_asm already forbids other operands, so it shouldn't even be much trouble to implement. And speaking personally, in our own codebase that would suffice to allow us to completely remove this feature gate.

@roblabla
Copy link
Contributor

That was suggested during the stabilization of asm, but rejected because:

The current implementation of sym will likely need to be rewritten to support both asm! and global_asm!, which may cause subtle breaking changes (in particular with regards to functions with generic parameters).

@eddyb
Copy link
Member

eddyb commented Feb 24, 2022

The way sym is handled by the compiler (in HIR & typeck) is likely to change in the future to support it in global_asm. Currently it's a Path that's evaluated in the typeck context of the function containing the asm! but that doesn't work with global_asm! which exists outside of a function body.

IMO the way to do this is to require an AnonConst (similar to the const {...} feature) around the sym path, at least in global_asm!. While it might be possible to whip up a typeck session without that, it seems easier to reuse the AnonConst functionality (similar to how we would implement typeof(...)).

Only downside I can think of is that it's a bit annoying that a HIR body containing a path to a static looks like it's reading from it (because static paths are place expressions instead of value expressions) - might be bypassed by "simply" not running actually running any MIR processing on it (including MIR borrowck), I'm not sure.

which may cause subtle breaking changes (in particular with regards to functions with generic parameters).

I would not want to disallow anything that works in paths within a function, seems like an artificial limitation when there is clearly an implementation strategy that works (i.e. the current one), and global_asm! is generally more limited anyway (making identical implementation between the two questionable).

@Amanieu
Copy link
Member Author

Amanieu commented Mar 1, 2022

#94468 adds sym support for global_asm! which should help unblock the stabilization of sym.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 10, 2022
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`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 10, 2022
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``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 14, 2022
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`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 16, 2022
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`
@bstrie
Copy link
Contributor

bstrie commented Apr 16, 2022

Now that #94468 has landed, is all that's left to make a stabilization report?

@Amanieu
Copy link
Member Author

Amanieu commented Apr 18, 2022

Stabilization report

This is a stabilization request for the asm_sym feature.

Description

This feature adds a sym <path> operand type to asm! and global_asm!.

  • <path> must refer to a fn or static.
  • A mangled symbol name referring to the item is substituted into the asm template string.
  • The substituted string does not include any modifiers (e.g. GOT, PLT, relocations, etc).
  • <path> is allowed to point to a #[thread_local] static, in which case the asm code can combine the symbol with relocations (e.g. @plt, @TPOFF) to read from thread-local data.

Example

extern "C" fn add(x: i32, y: i32) -> i32 {
    x + y
}

// AArch64
let result: i32;
asm!(
    "call {}",
    sym add,
    in("x0") 1,
    in("x1") 2,
    lateout("x0") result,
    clobber_abi("C"),
);
assert_eq!(result, 3);

Tests

This feature is tested in:

  • src/test/ui/asm
  • src/test/assembly/asm
  • src/test/codegen/asm-*

flip1995 pushed a commit to flip1995/rust that referenced this issue Apr 21, 2022
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`
@Lokathor
Copy link
Contributor

For those you'd have to use a bunch of cfg the same as C would (apparently) use a bunch of macros.

and the cfg would have to go by a cargo feature or other expression that says what the linking mode will be.

@marcgalois
Copy link

Would it be possible to expose the code model and relocation model via cfg? If so, then would it be possible to write a macro to generate all the #[cfg(...)] statements to generate the proper assembly for getting the address of a symbol?

@joshtriplett
Copy link
Member

@Amanieu wrote:

Inline assembly is inherently target specific.

It's inherently architecture-specific; it doesn't necessarily have to be OS-specific.

@joshtriplett joshtriplett added the S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR label Aug 10, 2022
@nikomatsakis
Copy link
Contributor

@rfcbot concern rust-reference

Is there a PR adding this to the rust reference?

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@Amanieu
Copy link
Member Author

Amanieu commented Aug 16, 2022

No, but the addition is pretty small. It's basically a revert of this.

@nikomatsakis
Copy link
Contributor

@Amanieu great! Can you open a PR with that revert?

@Amanieu
Copy link
Member Author

Amanieu commented Sep 16, 2022

rust-lang/reference#1270

@nikomatsakis
Copy link
Contributor

@rfcbot resolve rust-reference

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 20, 2022
@rfcbot
Copy link

rfcbot commented Sep 20, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 30, 2022
@rfcbot
Copy link

rfcbot commented Sep 30, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@hudson-ayers
Copy link
Contributor

It sounds like all that is left is for an actual stabilization PR to be submitted?

@Amanieu
Copy link
Member Author

Amanieu commented Oct 17, 2022

It sounds like all that is left is for an actual stabilization PR to be submitted?

#103168

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 18, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 18, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 18, 2022
@SUPERCILEX
Copy link
Contributor

Shouldn't this issue be closed since the feature was stabilized?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: inline asm!(..) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-asm `#![feature(asm)]` (not `llvm_asm`) finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests