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

fix ICE in CMSE type validation #130064

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Conversation

folkertdev
Copy link
Contributor

fixes #129983

tracking issue: #81391

r? @compiler-errors

@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 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2024

HIR ty lowering was modified

cc @fmease

@@ -67,13 +70,13 @@ pub(crate) fn validate_cmse_abi<'tcx>(
/// Returns whether the inputs will fit into the available registers
fn is_valid_cmse_inputs<'tcx>(
tcx: TyCtxt<'tcx>,
fn_sig: ty::PolyFnSig<'tcx>,
fn_sig: ty::FnSig<'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

One tweak -- can you change this back to PolyFnSig and do the binder instantiate call inside this fn?

for (index, arg_def) in fn_sig.inputs().iter().enumerate() {
let layout = tcx.layout_of(ParamEnv::reveal_all().and(*arg_def.skip_binder()))?;
for (index, ty) in fn_sig.inputs().iter().enumerate() {
let layout = tcx.layout_of(ParamEnv::reveal_all().and(*ty))?;
Copy link
Member

Choose a reason for hiding this comment

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

Side-note: this param-env is not right, so it'll probably ICE in other cases...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, #130104 was just reported and is not fixed by this branch (currently). How do we get a correct ParamEnv here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually that second ICE is due to encountering Infer being considered a bug in layout computation

ty::Bound(..) | ty::CoroutineWitness(..) | ty::Infer(_) | ty::Error(_) => {
bug!("Layout::compute: unexpected type `{}`", ty)
}
ty::Placeholder(..) | ty::Param(_) => {
return Err(error(cx, LayoutError::Unknown(ty)));
}

Moving ty::Infer(_) to the ty::Placeholder(..) branch fixes that problem, but might hide bugs in other cases?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that is 100% not the right fix. the problem here is that we need to delay the calculation of these layouts until after type-checking is done for types that show up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here #127814 (comment) is where some discussion around the current design happened. Is there an existing way to perform that traversal after type checking to perform this layout check?

@compiler-errors
Copy link
Member

pls in the future squash changes like this into one commit, a one-liner is not really worth the additional commit lol

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 9, 2024

📌 Commit e186cc6 has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Sep 9, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 9, 2024
…piler-errors

fix ICE in CMSE type validation

fixes rust-lang#129983

tracking issue: rust-lang#81391

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129929 (`rustc_mir_transform` cleanups, round 2)
 - rust-lang#130022 (Dataflow/borrowck lifetime cleanups)
 - rust-lang#130064 (fix ICE in CMSE type validation)
 - rust-lang#130067 (Remove redundant check in `symlink_hard_link` test)
 - rust-lang#130131 (Print a helpful message if any tests were skipped for being up-to-date)
 - rust-lang#130137 (Fix ICE caused by missing span in a region error)
 - rust-lang#130153 (use verbose flag as a default value for `rust.verbose-tests`)
 - rust-lang#130154 (Stabilize `char::MIN`)
 - rust-lang#130158 (Update books)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1490fe6 into rust-lang:master Sep 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2024
Rollup merge of rust-lang#130064 - folkertdev:fix-issue-129983, r=compiler-errors

fix ICE in CMSE type validation

fixes rust-lang#129983

tracking issue: rust-lang#81391

r? ``@compiler-errors``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: has escaping bound vars, so it cannot be wrapped in a dummy binder
4 participants