Skip to content

Do not ICE when calling incorrectly defined transmute intrinsic #123526

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 6, 2024
Merged

Do not ICE when calling incorrectly defined transmute intrinsic #123526

merged 1 commit into from
Apr 6, 2024
+42 −2

Conversation

@estebank
Copy link
Contributor

@estebank estebank commented Apr 6, 2024

Fix #123442

@rustbot
Copy link
Collaborator

rustbot commented Apr 6, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Apr 6, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Apr 6, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 6, 2024

📌 Commit aa53bc0 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 Apr 6, 2024
@compiler-errors compiler-errors mentioned this pull request Apr 6, 2024
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 6, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
@bors
Auto merge of rust-lang#123540 - matthiaskrgr:rollup-8ewq0zt, r=matth…
3f10032 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123294 (Require LLVM_CONFIG to be set in rustc_llvm/build.rs)
 - rust-lang#123467 (MSVC targets should use COFF as their archive format)
 - rust-lang#123498 (explaining `DefKind::Field`)
 - rust-lang#123519 (Improve cfg and check-cfg configuration)
 - rust-lang#123525 (CFI: Don't rewrite ty::Dynamic directly)
 - rust-lang#123526 (Do not ICE when calling incorrectly defined `transmute` intrinsic)
 - rust-lang#123528 (Hide async_gen_internals from standard library documentation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc2dbbb into rust-lang:master Apr 6, 2024
@rustbot rustbot added this to the 1.79.0 milestone Apr 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
@rust-timer
Unrolled build for rust-lang#123526
5efc43b 
Rollup merge of rust-lang#123526 - estebank:issue-123442, r=compiler-errors

Do not ICE when calling incorrectly defined `transmute` intrinsic

Fix rust-lang#123442
@Noratrieb
Copy link
Member

Noratrieb commented Apr 7, 2024

why was this change done? it is a fairly simple change but also completely unnecessary, anyone defining transmute with the wrong params deserves am ICE

@estebank
Copy link
Contributor Author

estebank commented Apr 7, 2024

@Nilstrieb I know that as a policy we won't go out of our way to avoid ICEs caused by incorrect intrinsics, but I do believe that when handling that is a small enough amount of code, we should. I have three reasons for that position:

  • Polish: ICEs leave a bad taste in our users' mouth. They might leave with a perception that the compiler is a worse codebase than it really is. "You're holding it wrong" is not a good reason for ICEs.
  • Practical: every ICE tells people that we expect them to file a ticket. ICEing on things that are not compiler bugs means we'll receive tickets in our issue tracker that are unnecessary.
  • Speculative: if we ever have a long-running rustc service, or rust-analyzer relies on it, or any number of other tooling changes that require certain levels of correct behavior from rustc happen, then unnecessary ICEs could cause incorrect behavior.

I think all three points (or at least the first two) can be handled in two ways: either we handle the invalid state with delay_span_bug or we introduce a new, separate, version of panicking that doesn't "look" like an ICE but rather a generic "invalid language state" error that stops the compilation, with no stack trace.

@estebank estebank mentioned this pull request Apr 7, 2024
Closed
@compiler-errors compiler-errors mentioned this pull request Apr 7, 2024
Merged
fmease added a commit to fmease/rust that referenced this pull request Apr 15, 2024
@fmease
Rollup merge of rust-lang#123603 - compiler-errors:no-intrinsic, r=es…
1ad9fea 
…tebank

Don't even parse an intrinsic unless the feature gate is enabled

Don't return true in `tcx.is_intrinsic` if the function is defined locally and `#![feature(intrinsics)]` is not enabled. This is a slightly more general fix than rust-lang#123526, since rust-lang#123587 shows that we have simplifying assumptions about intrinsics elsewhere in the compiler.

This will make the code ICE again if the user **enables** `#[feature(intrinsics)]`, but I kind of feel like if we want to fix that, we should make the `INTERNAL_FEATURES` lint `Deny` again. Perhaps we could do that on non-nightly compilers. Or we should just stop compilation altogether if they have `#![feature]` enabled on a non-nightly compiler.

As for the UX of *real* cases of hitting these ICEs, I believe pretty strongly that if a compiler/stdlib dev is modifying internal intrinsics (intentionally, like when making a change to rustc) we have no guarantee to make the ICE better looking for them. Honestly, *not* spitting out a stack trace is probably a disservice to the people who hit those ICEs in that case.

r? `@Nilstrieb` `@estebank`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2024
@rust-timer
Unrolled build for rust-lang#123603
1a7b16b 
Rollup merge of rust-lang#123603 - compiler-errors:no-intrinsic, r=estebank

Don't even parse an intrinsic unless the feature gate is enabled

Don't return true in `tcx.is_intrinsic` if the function is defined locally and `#![feature(intrinsics)]` is not enabled. This is a slightly more general fix than rust-lang#123526, since rust-lang#123587 shows that we have simplifying assumptions about intrinsics elsewhere in the compiler.

This will make the code ICE again if the user **enables** `#[feature(intrinsics)]`, but I kind of feel like if we want to fix that, we should make the `INTERNAL_FEATURES` lint `Deny` again. Perhaps we could do that on non-nightly compilers. Or we should just stop compilation altogether if they have `#![feature]` enabled on a non-nightly compiler.

As for the UX of *real* cases of hitting these ICEs, I believe pretty strongly that if a compiler/stdlib dev is modifying internal intrinsics (intentionally, like when making a change to rustc) we have no guarantee to make the ICE better looking for them. Honestly, *not* spitting out a stack trace is probably a disservice to the people who hit those ICEs in that case.

r? `@Nilstrieb` `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@pnkfelix pnkfelix

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

Milestone

1.79.0

Development

Successfully merging this pull request may close these issues.

expr.rs index out of bounds: the len is 0 but the index is 0

6 participants

@estebank @rustbot @compiler-errors @bors @Noratrieb @pnkfelix