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

Handle lifetimes correctly in sym in inline and global asm #116087

Closed
wants to merge 3 commits into from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Sep 23, 2023

Fixes #111709
Fixes #96304

sym in global-asm can never reference regions other than 'static, so it suffices to fold the constant and replace erased regions with 'static (see first commit). However, we want to be able to mention both early- and late-bound (and local, e.g. #111709) regions from the function in inline-asm.

This change is a bit invasive, but I'm not really convinced it can be simpler, at least without having to teach global_asm! how to be an inference body or something.

This conceptually reverts part of dc345d8 for inline asm, insofar as we return to treating SymFn in non-global_asm bodies as a regular HIR exprs.

cc @Amanieu who stabilized sym in #93333

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 23, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors changed the title Handle lifetimes correctly in in sym in inline and global asm Handle lifetimes correctly in sym in inline and global asm Sep 23, 2023
@@ -128,7 +128,7 @@ pub(super) fn generics_of(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Generics {
Node::Expr(&Expr { kind: ExprKind::InlineAsm(asm), .. })
if asm.operands.iter().any(|(op, _op_sp)| match op {
hir::InlineAsmOperand::Const { anon_const }
| hir::InlineAsmOperand::SymFn { anon_const } => {
| hir::InlineAsmOperand::SymFnInGlobal { anon_const } => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not reachable here since this is only reached for ExprKind::InlineAsm, not `ItemKind::GlobalAsm``.

@Amanieu
Copy link
Member

Amanieu commented Sep 23, 2023

Why is it necessary to change SymFnInInline to no longer use an AnonConst? Aren't inline const blocks able to properly handle lifetimes?

@compiler-errors
Copy link
Member Author

inline consts (const {}) can handle lifetimes, but sym are not represented as inline consts -- theyre AnonConst, which act somewhat differently. They're separate items, so they are type checked totally separately from their enclosing item. Inline consts act more like closures where they're type checked together with their enclosing body.

@bors
Copy link
Contributor

bors commented Oct 7, 2023

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

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 12, 2023

inline consts (const {}) can handle lifetimes, but sym are not represented as inline consts -- theyre AnonConst, which act somewhat differently. They're separate items, so they are type checked totally separately from their enclosing item. Inline consts act more like closures where they're type checked together with their enclosing body.

Is anon const actually the desirable semantics here, or it just happens to be the current semantics?
I suspect that references in inline asm are an uncommon corner case, and we can change it if necessary.

at least without having to teach global_asm! how to be an inference body or something.

That's what I'd expect to do.
If the piece of code is an expression, that may have generic arguments and lifetimes in it (and value path is such an expression), then it should be represented as a body, no? Regardless of whether it's a global or local asm.

@petrochenkov petrochenkov 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 Oct 12, 2023
@compiler-errors
Copy link
Member Author

If the piece of code is an expression, that may have generic arguments and lifetimes in it (and value path is such an expression), then it should be represented as a body, no? Regardless of whether it's a global or local asm.

This is not possible at the moment. Global-asm aren't type-checked in the sense of HIR bodies, nor are they represented with a HIR body.

I'd rather just altogether deny lifetime arguments in sym, if both of y'all are unconvinced by the complexity of this approach.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2023
@petrochenkov
Copy link
Contributor

This is not possible at the moment. Global-asm aren't type-checked in the sense of HIR bodies, nor are they represented with a HIR body.

What do you mean by not possible?
If e.g. enum discriminants can be bodies, then symbols in global asm certainly could as well.

I'd rather just altogether deny lifetime arguments in sym

Why it it ok to have type arguments, but not lifetime arguments?
I'm actually not sure how the path expressions in sym are type checked if they are not bodies, but they are indeed type checked at least to some extent because this fails

fn foo<T: Copy>() {}

sym foo::<NonCopy>

The question about anon consts vs inline consts also wasn't answered.
@rustbot author

@rustbot rustbot 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 Oct 26, 2023
@compiler-errors
Copy link
Member Author

enum discriminants can be bodies, then symbols in global asm certainly could as well.

That's because enum discriminants actually correspond to expressions in HIR.

Why it it ok to have type arguments, but not lifetime arguments?

Because we explicitly erase lifetime arguments during writeback in HIR.

I'm actually not sure how the path expressions in sym are type checked if they are not bodies, but they are indeed type checked at least to some extent because this fails

They are bodies. They just don't have a parent body, which is required for inline-consts to make sense. Inline consts are type-checked in the parent body, like closures. It's like having a closure with no enclosing body -- it doesn't make sense.

@petrochenkov: Remind me what specific question about inline you want answered, though?

@petrochenkov
Copy link
Contributor

Remind me what specific question about inline you want answered, though?

What would be the preferable semantics for sym - inline const or anon const (if backward compatibility, etc is out of question).

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 26, 2023

That's because enum discriminants actually correspond to expressions in HIR.

Okay, then sym should correspond to expressions in HIR.
SymFnInInline does correspond to an expression, SymFnInGlobal should do that too, I don't see why they should diverge.

Yes, SymFnInGlobal is an expression in a "global" context, outside of any parent body, but that's exactly the same situation as with const initializers or enum variant discriminants, if there's no top level body then we should create it.

@compiler-errors
Copy link
Member Author

Yes, SymFnInGlobal is an expression in a "global" context, outside of any parent body, [...] if there's no top level body then we should create it.

I predict this is going to be prohibitively complicated.

but that's exactly the same situation as with const initializers or enum variant discriminants

This is not exactly the same situation as const initializers or enum variant discriminants, since they never have the possibility of participating in region inference with infer lifetimes that may be inherited from the parent, nor can they reference late-bound regions (which are very very broken, even with #[feature(generic_const_expressions)], cc #115486).

Their restrictions means that it's okay for them to be represented as an AnonConst, which only allows them to reference lifetimes from the parent in certain situations, and even then, only when generic_const_exprs is enabled due to the way we set up the generics for an anon-const.

@compiler-errors
Copy link
Member Author

compiler-errors commented Nov 17, 2023

Don't care to work on this anymore. inline_asm is still super broken when lifetimes are involved, and I still maintain that it needs a more invasive fix like this, for the reasons I elaborated above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants