Skip to content

fix ICE in CMSE type validation #130064

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
Collaborator

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``
# for free to join this conversation on GitHub. Already have an account? # 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