cmse: disallow impl Trait in cmse-nonsecure-entry return types
#147243
Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
|
r? @davidtwco @bors r+ rollup |
bors
added
S-waiting-on-bors
|
@bors r- Sorry, for the late r- I only looked at the contents right now (I'm subscribed due to it modifying HIR ty lowering). |
bors
added
S-waiting-on-author
folkertdev
force-pushed
the
cmse-bail-impl-trait
branch
from
3571274 to
fbdc685
Compare
|
Thanks for addressing my concern! @bors r=davidtwco |
bors
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - #145943 (stdlib docs: document lifetime extension for `format_args!`'s arguments) - #147243 (cmse: disallow `impl Trait` in `cmse-nonsecure-entry` return types) - #147402 ([rustdoc] Don't serialize & deserialize data that doesn't go OTW) - #147418 (Fix target list of `link_section`) - #147429 (Print tip for human error format in runtest) - #147441 (Fix comments error for Provenance impls) - #147442 (c-variadic: fix thir-print for `...` without a pattern) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #147243 - folkertdev:cmse-bail-impl-trait, r=davidtwco cmse: disallow `impl Trait` in `cmse-nonsecure-entry` return types tracking issue: #75835 fixes #147242 Refactors some logic to be more robust in the future, and then disallows `impl Trait` as a return type for the cmse ABIs. The `is_valid_cmse_output_layout` function disallows `union` values like before. That is not entirely correct, but preserves the current behavior. Some additional logic is needed for `union` values (and any types where parts may be uninitialized) that I'll tackle in a later PR. can be reviewed commit-by-commit. r? types
| // Here we explicitly disallow `impl Trait` in the `cmse-nonsecure-entry` return type too, to | ||
| // prevent query cycles when calculating the layout. This ABI is meant to be used with | ||
| // `#[no_mangle]` or similar, so generics in the type really don't make sense. |
There was a problem hiding this comment.
for the record opaque types are not generics. fn foo() -> impl Trait's return type is some fully concrete non generic type, its just not explicitly written out. I think you're right that no_mangle'd functions with RPITs are silly though
There was a problem hiding this comment.
really? My intuition is that -> impl Trait is polymorphic and that it is roughly equivalent to fn foo<T: Trait>() -> T. You'd need to pick a concrete imlementation of the trait in order to actually be able to calculate the layout of the return type (and from that the ABI).
Is there some better word for "the type is not known enough to calculate the layout"?
There was a problem hiding this comment.
the type of RPITs should be known enough to calculate the layout of the return type as long as opaque types are able to be properly normalized. impl Trait only desugars to a generic parameter in argument position, i.e. fn foo(a: impl Trait) desugars as you say
There was a problem hiding this comment.
Oh interesting, it actually looks at the body to figure out the concrete type if it occurs just in return position
There was a problem hiding this comment.
But at the point where we attempt to generate the layout for these entry functions, somehow there is a loop in the calculation
https://godbolt.org/z/K58sMorf3
(that godbolt only works with current nightly, but the same cycle is in the issue #147242)
anyway we're totally fine with just disallowing impl Trait entirely, it's a simple rule that doesn't really limit any practical use.
There was a problem hiding this comment.
yeah I would be interested in seeing where the cycle comes from, but it does seem fine to just altogether forbid this as RPITs in no_mangle functions seems like a big footgun/code smell
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#145943 (stdlib docs: document lifetime extension for `format_args!`'s arguments) - rust-lang#147243 (cmse: disallow `impl Trait` in `cmse-nonsecure-entry` return types) - rust-lang#147402 ([rustdoc] Don't serialize & deserialize data that doesn't go OTW) - rust-lang#147418 (Fix target list of `link_section`) - rust-lang#147429 (Print tip for human error format in runtest) - rust-lang#147441 (Fix comments error for Provenance impls) - rust-lang#147442 (c-variadic: fix thir-print for `...` without a pattern) r? `@ghost` `@rustbot` modify labels: rollup
tracking issue: #75835
fixes #147242
Refactors some logic to be more robust in the future, and then disallows
impl Traitas a return type for the cmse ABIs.The
is_valid_cmse_output_layoutfunction disallowsunionvalues like before. That is not entirely correct, but preserves the current behavior. Some additional logic is needed forunionvalues (and any types where parts may be uninitialized) that I'll tackle in a later PR.can be reviewed commit-by-commit.
r? types