-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
#[inline(required)]
#3711
#[inline(required)]
#3711
Conversation
I like the idea generally. Two obvious questions occur to me that I don't see addressed in this RFC though.
First question is, why are these intended to remain unstable definitely? One of the motivations is for security, but it seems like user code could benefit from that too. And I've also often wanted "inline this function or yell at me" when writing SIMD code. I use Second question is, why does this propose both a |
I'll update the RFC shortly to address these questions too.
Just to avoid it being put on items unnecessarily in an attempt to improve performance. I spoke with some other contributors at EuroRust last week about this and we felt that was a reasonable concern. I expect that we'd put these attributes on SIMD intrinsics in std and places like that, so users of those intrinsics would get warnings if they weren't inlined (even if they can't use these attributes themselves). I don't feel strongly about this detail though, if there were reasonable cases where one might want to annotate user code with this then we can make it available to users.
This is an alternative that we could consider, but I went with having two lints and two attributes because of the use case where we require inlining to uphold security properties, I wanted that to be a hard error. I wouldn't want a toolchain version bump resulting in a security intrinsic no longer being inlined and that going unnoticed in CI and compromising the security of an application. You're absolutely right that we could just make this a warning, and ask that users of any security intrinsics add the |
I think that it's substantially more confusing to have So, to me:
And while, yes, |
I'm definitely open to renaming
I'm also open to something like this too if there's consensus. I didn't go with something like this initially because I felt having two new inlining kinds and only having those unconditionally run the inliner seemed tidier. |
I mean, as far as I'm concerned, it's exactly the same thing, just slightly wordier. And, I'd prefer wordiness over confusion. To me, seeing |
I'd think Is there a reasons why |
A different solution could be to rename |
Cc @saethlin who was working on |
Regarding user-controlled "try your best to inline this and warn me if you can't" annotations for performance reasons - I've also wished for something like this in the past. However, I think there's two subtly different use cases behind this: It would be nice to get warnings if On the other hand, sometimes I really do want to force inlining... when building with optimizations. Most recently, I've had to slap |
rely on being inlined to guarantee their security properties. Rust should make | ||
it easier to guarantee that these intrinsics will be inlined and emit an error | ||
if that is not possible or has not occurred. | ||
- Arm's Neon and SVE performance intrinsics have work poorly if not inlined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly what words you meant here, but "have work poorly" is not it.
- Arm's Neon and SVE performance intrinsics have work poorly if not inlined. | |
- Arm's Neon and SVE performance intrinsics perform quite poorly if not inlined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yeah, just missed that while rewriting this sentence at some point.
In general, for many intrinsics, inlining is not an optimisation but an | ||
important part of the semantics, regardless of optimisation level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you truly do mean that inlining is part of the semantics, then I think a lot of work needs to be done here, including making this "inlining" happen for the MIR that Miri executes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming I'm reading this and the draft implementation right, -Zmir-enable-passes=-Inline
in the draft PR is unsound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my wording was too strong here, there wouldn't necessarily be unsoundness, but for something like SIMD intrinsics, they basically only make sense if you inline them - it isn't wrong if you don't, but it isn't more debuggable or anything like that, just slower. These intrinsics only make sense inlined.
warn-by-default lint and `#[inline(required)]` emits a deny-by-default lint, | ||
which can be used by items which must be inlined to uphold security properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that a deny-by-default lint is sufficiently stern for upholding security properties, especially if the point of this RFC is that auditing the assembly is not an option. A deny-by-default lint is silent when it fires in a dependency, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear if anyone outside of a crypto library would ever use this for "security". Does an everyday user need to be forcing inlining when writing a webserver or whatever the heck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that a deny-by-default lint is sufficiently stern for upholding security properties, especially if the point of this RFC is that auditing the assembly is not an option. A deny-by-default lint is silent when it fires in a dependency, right?
Yeah, you need to trust your dependencies which use inline(required)
functions to not just #[allow(..)]
'ing those lints. I made it a lint so that you could allow/expect it yourself if limitations in the inliner were causing the lint to trigger and there was nothing you could do about that.
I'm unclear if anyone outside of a crypto library would ever use this for "security". Does an everyday user need to be forcing inlining when writing a webserver or whatever the heck?
See #3711 (comment) for an example of the type of function where inline(required)
would be intended to be used. I don't anticipate that the average user would want to be using this to write their webserver, it's to support a very specific thing, which is why I'm entirely open to it being a rustc_must_inline
attribute or being perma-unstable or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not trying to be too concerned about the process of it all, but would this idea better fit a Compiler MCP, rather than a general RFC? If it's intended to just eternally be some implementation detail. Generally, users expect RFCs to result in something that they can eventually use on stable. That also might reduce the bikeshed.
As @hanna-kruppe points out, I think the existing inline attributes are a very poor fit for how the attributes are used in practice.
Normal MIR inlining doesn't run in That being said, I have serious concerns about using the MIR inliner for this. The MIR inliner isn't able to resolve all calls, for a few reasons. When it fails to resolve a call, we can't know if that was a The RFC says "Failures to force inline should not occur sporadically," but to the best of my knowledge, the query system requires the MIR inliner to have query cycle avoidance logic which is based on analyzing unoptimized MIR. Calls that are optimized out in MIR can cause a cycle to be detected and avoided by not doing some inlining. That seems bad. I suspect there are other problems. Have you considered changing MIR building or codegen to correctly lower calls to these must-be-inlined functions instead of leaning on the MIR inliner? How do you plan to handle indirect calls to functions with these attributes? Can we ban coercing these functions to function pointers? |
That's the first I've heard of this problem. I would encourage you to file an issue. But as the author of aho-corasick, I generally do not apply inline(always) unless it's supported by a benchmark or profiling. I don't just sprinkle them about when I feel like it. |
already implemented by Rust. Explicit pointer authentication intrinsics are not | ||
yet available in Rust. Exposing pointer authentication intrinsics would require | ||
this RFC, or something providing equivalent guarantees, as a prerequisite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain or provide a link to why these pointer authentication intrinsics require inlining for correctness?
Also my knee-jerk reaction to this need is to... make them actual intrinsics. And not even add the attribute this RFC proposes. If the attribute is going to be perma-unstable/internal-only, what's the difference between that and a lang item?
I do not find this use-case compelling, and I'd much rather the RFC be motivated by something that we can't do using a well-established pattern based on lang items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a link, but for pointer authentication, the idea would be to upstream some functions into stdarch which use inline assembly1 and must be inlined. They have to be inlined because otherwise you're left with a sequence of instructions that can be used as a gadget to sign whatever you want, so you want to mix those sequences into the functions so they can't be used as a gadget.
For these functions, inline(required)
would be necessary to make sure that if you're using these intrinsics that they actually have a benefit. The important part of inline(required)
for this case is really the diagnostic, inline(always)
gives us about as much inlining. If there's a more limited form of this proposal which - for just these intrinsics - can give us the diagnostic, then that works too.
Footnotes
-
LLVM does have intrinsics that for pointer authentication, but using inline assembly gives us more guarantees around upholding the security properties of the pointer authentication, e.g. calls to identical intrinsics can be optimised away, unencrypted values could be spilled to memory briefly, and using inline assembly can avoid these issues. ↩
To be clear, I was referring to (1) the current behavior of |
I think that the performance motivation part of this (and consequently |
That makes sense, the MIR inliner may not be a viable approach, at least for a vaguely general-purpose attribute like this RFC currently proposes. |
Not making them available to users makes the feature non-compositional: I can't write a wrapper function around a |
@davidtwco If this is intended to be perma-unstable, my take is that no RFC is needed. This is an implementation detail for how rustc manages certain intrinsic functions, and it should just use an attribute like |
Thanks everyone for the comments! I'm no longer convinced that the approach described in this RFC is feasible and I think I've got alternative solutions to address the motivating issues that I'll pursue instead, so closing for now. |
mir_transform: implement `#[rustc_force_inline]` Adds `#[rustc_force_inline]` which is similar to always inlining but reports an error if the inlining was not possible. - `#[rustc_force_inline]` can only be applied to free functions to guarantee that the MIR inliner will be able to resolve calls. - `rustc_mir_transform::inline::Inline` is refactored into two passes (`Inline` and `ForceInline`), sharing the vast majority of the implementation. - `rustc_mir_transform::inline::ForceInline` can't be disabled so annotated items are always inlined. - `rustc_mir_transform::inline::ForceInline` runs regardless of optimisation level. - `#[rustc_force_inline]` won't inline unless target features match, as with normal inlining. - MIR validation will ICE if a `#[rustc_force_inline]` isn't inlined, to guarantee that it will never be codegened independently. As a further guarantee, monomorphisation collection will always decide that `#[rustc_force_inline]` functions cannot be codegened locally. - Like other intrinsics, `#[rustc_force_inline]` annotated functions cannot be cast to function pointers. - As with other rustc attrs, this cannot be used by users, just within the compiler and standard library. - This is only implemented within rustc, so should avoid any limitations of LLVM's inlining. It is intended that this attribute be used with intrinsics that must be inlined for security reasons. For example, pointer authentication intrinsics would allow Rust users to make use of pointer authentication instructions, but if these intrinsic functions were in the binary then they could be used as gadgets with ROP attacks, defeating the point of introducing them. We don't have any intrinsics like this today, but I expect to upstream some once a force inlining mechanism such as this is available. cc rust-lang#131687 rust-lang/rfcs#3711 - this approach should resolve the concerns from these previous attempts r? `@saethlin`
mir_transform: implement `#[rustc_force_inline]` Adds `#[rustc_force_inline]` which is similar to always inlining but reports an error if the inlining was not possible. - `#[rustc_force_inline]` can only be applied to free functions to guarantee that the MIR inliner will be able to resolve calls. - `rustc_mir_transform::inline::Inline` is refactored into two passes (`Inline` and `ForceInline`), sharing the vast majority of the implementation. - `rustc_mir_transform::inline::ForceInline` can't be disabled so annotated items are always inlined. - `rustc_mir_transform::inline::ForceInline` runs regardless of optimisation level. - `#[rustc_force_inline]` won't inline unless target features match, as with normal inlining. - MIR validation will ICE if a `#[rustc_force_inline]` isn't inlined, to guarantee that it will never be codegened independently. As a further guarantee, monomorphisation collection will always decide that `#[rustc_force_inline]` functions cannot be codegened locally. - Like other intrinsics, `#[rustc_force_inline]` annotated functions cannot be cast to function pointers. - As with other rustc attrs, this cannot be used by users, just within the compiler and standard library. - This is only implemented within rustc, so should avoid any limitations of LLVM's inlining. It is intended that this attribute be used with intrinsics that must be inlined for security reasons. For example, pointer authentication intrinsics would allow Rust users to make use of pointer authentication instructions, but if these intrinsic functions were in the binary then they could be used as gadgets with ROP attacks, defeating the point of introducing them. We don't have any intrinsics like this today, but I expect to upstream some once a force inlining mechanism such as this is available. cc rust-lang#131687 rust-lang/rfcs#3711 - this approach should resolve the concerns from these previous attempts r? `@saethlin`
mir_transform: implement `#[rustc_force_inline]` Adds `#[rustc_force_inline]` which is similar to always inlining but reports an error if the inlining was not possible. - `#[rustc_force_inline]` can only be applied to free functions to guarantee that the MIR inliner will be able to resolve calls. - `rustc_mir_transform::inline::Inline` is refactored into two passes (`Inline` and `ForceInline`), sharing the vast majority of the implementation. - `rustc_mir_transform::inline::ForceInline` can't be disabled so annotated items are always inlined. - `rustc_mir_transform::inline::ForceInline` runs regardless of optimisation level. - `#[rustc_force_inline]` won't inline unless target features match, as with normal inlining. - MIR validation will ICE if a `#[rustc_force_inline]` isn't inlined, to guarantee that it will never be codegened independently. As a further guarantee, monomorphisation collection will always decide that `#[rustc_force_inline]` functions cannot be codegened locally. - Like other intrinsics, `#[rustc_force_inline]` annotated functions cannot be cast to function pointers. - As with other rustc attrs, this cannot be used by users, just within the compiler and standard library. - This is only implemented within rustc, so should avoid any limitations of LLVM's inlining. It is intended that this attribute be used with intrinsics that must be inlined for security reasons. For example, pointer authentication intrinsics would allow Rust users to make use of pointer authentication instructions, but if these intrinsic functions were in the binary then they could be used as gadgets with ROP attacks, defeating the point of introducing them. We don't have any intrinsics like this today, but I expect to upstream some once a force inlining mechanism such as this is available. cc rust-lang#131687 rust-lang/rfcs#3711 - this approach should resolve the concerns from these previous attempts r? `@saethlin`
mir_transform: implement `#[rustc_force_inline]` Adds `#[rustc_force_inline]` which is similar to always inlining but reports an error if the inlining was not possible. - `#[rustc_force_inline]` can only be applied to free functions to guarantee that the MIR inliner will be able to resolve calls. - `rustc_mir_transform::inline::Inline` is refactored into two passes (`Inline` and `ForceInline`), sharing the vast majority of the implementation. - `rustc_mir_transform::inline::ForceInline` can't be disabled so annotated items are always inlined. - `rustc_mir_transform::inline::ForceInline` runs regardless of optimisation level. - `#[rustc_force_inline]` won't inline unless target features match, as with normal inlining. - MIR validation will ICE if a `#[rustc_force_inline]` isn't inlined, to guarantee that it will never be codegened independently. As a further guarantee, monomorphisation collection will always decide that `#[rustc_force_inline]` functions cannot be codegened locally. - Like other intrinsics, `#[rustc_force_inline]` annotated functions cannot be cast to function pointers. - As with other rustc attrs, this cannot be used by users, just within the compiler and standard library. - This is only implemented within rustc, so should avoid any limitations of LLVM's inlining. It is intended that this attribute be used with intrinsics that must be inlined for security reasons. For example, pointer authentication intrinsics would allow Rust users to make use of pointer authentication instructions, but if these intrinsic functions were in the binary then they could be used as gadgets with ROP attacks, defeating the point of introducing them. We don't have any intrinsics like this today, but I expect to upstream some once a force inlining mechanism such as this is available. cc rust-lang#131687 rust-lang/rfcs#3711 - this approach should resolve the concerns from these previous attempts r? `@saethlin`
rustc supports inlining functions with the
#[inline]
/#[inline(always)]
/#[inline(never)]
attributes, but these are only hints to the compiler.Sometimes it is desirable to require inlining, not just suggest inlining: security APIs can depend on being inlined for their security properties, and performance intrinsics can work poorly if not inlined.
Add a forever-unstable
#[inline(required)]
and#[inline(must)]
which will always inline regardless of cost heuristics and will emit a deny/warn lint if inlining was not possible.See rust-lang/rust#131687 for draft implementation.
Rendered