mgca: Don't ICE when evaluating ValTrees that contain error constants - #158401
Conversation
|
HIR ty lowering was modified cc @fmease |
|
r? @camelid rustbot has assigned @camelid. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
The case reported in the original issue goes through lower_const_arg_struct(). I found that a similar ICE can also occur through lower_const_arg_tuple_call(), and this serves as a regression test for that case.
There was a problem hiding this comment.
Is it a similar ICE or literally the same ICE? I'm not sure if it makes sense to have multiple tests for it since they all have the same root cause.
There was a problem hiding this comment.
Ah, indeed. When I was investigating using the old fix approach, they appeared to be different paths causing ICE, but now I see these are indeed the same issue. Thanks.
There was a problem hiding this comment.
I'm not sure if this is the right place to be fixing the problem. Given that ValTrees can have arbitrary ty::Const, it feels like they should be able to have error constants in them.
@BoxyUwU do you have thoughts about the best way to handle this?
| branches: impl IntoIterator<Item = ty::Const<'tcx>>, | ||
| ty: Ty<'tcx>, | ||
| ) -> Const<'tcx> { | ||
| let branches: Vec<_> = branches.into_iter().collect(); |
There was a problem hiding this comment.
Hmm I don't love that this requires collecting the iterator, looking through all the branches, and then passing it on.
There was a problem hiding this comment.
I just saw this discussion. Please let me think about it for a while... https://rust-lang.zulipchat.com/#narrow/channel/260443-project-const-generics/topic/const.20generics.20talkies.2013.20jul.202026.201600pm/with/609875464
There was a problem hiding this comment.
No worries. The TLDR is that we should probably instead make other parts of the compiler handle errors inside valtrees gracefully, rather than avoiding creating valtrees that contain errors in an ad hoc way like this PR does.
There was a problem hiding this comment.
Thanks for waiting! A recent change required additional #![feature(macroless_generic_const_args)] to reproduce ICE in the added tests, so I added it and made the fix.
This comment has been minimized.
This comment has been minimized.
262a68f to
3642b25
Compare
This comment has been minimized.
This comment has been minimized.
3642b25 to
875afff
Compare
|
@rustbot ready |
875afff to
eb4943a
Compare
This comment has been minimized.
This comment has been minimized.
|
This PR changes a file inside |
d2c620d to
bce6199
Compare
There was a problem hiding this comment.
Is it a similar ICE or literally the same ICE? I'm not sure if it makes sense to have multiple tests for it since they all have the same root cause.
bce6199 to
db8797e
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
|
@bors r+ rollup |
mgca: Don't ICE when evaluating ValTrees that contain error constants Fixes rust-lang#154632 Fixes rust-lang#150969 `mir::Const::eval` now checks `error_reported()` and returns `Err` instead of converting such values to `ConstValue`.
mgca: Don't ICE when evaluating ValTrees that contain error constants Fixes rust-lang#154632 Fixes rust-lang#150969 `mir::Const::eval` now checks `error_reported()` and returns `Err` instead of converting such values to `ConstValue`.
…uwer Rollup of 23 pull requests Successful merges: - #162164 (Revert "Implement Debug for C-like enums with a concatenated string") - #160564 (volatile: allow accesses to non-AM memory to trap) - #161579 (suggest calling a fn item used as the iterator of a `for` loop) - #162044 (coverage: Resolve spans to file-coordinates in a separate step) - #162120 (Introduce `PerOwnerLoweringState`) - #162151 (Test itanium mangling of `f16` and `f128`) - #162162 (Don't special-case `!` in stability checks anymore) - #162181 (Remove wrong UnusedBraces lint for iterator loop in edition 2024 ) - #162187 (Rename `thir::ExprKind::Use` to `ValueExpr`) - #158401 (mgca: Don't ICE when evaluating ValTrees that contain error constants) - #159873 (fuchsia: Add safestack as a supported sanitizer for x86_64 fuchsia) - #161847 (Preserve visibility in nested macro import suggestions) - #161972 (Improve tests for `#[track_caller]` in async) - #162008 (Render the `box` pattern removal diagnostic more actionable & remove `box` expression recovery) - #162065 (std: don't reference `libc::O_NOFOLLOW` on VxWorks in `set_perm_nofollow`) - #162076 (docs(num): clarify conditions under which error occurs in `impl TryFrom<int> for int`) - #162152 (Revert "retrieve supported GCC targets from the sysroot") - #162153 (Prefer `LLVMGetVersion` for runtime info) - #162168 (fix ICE in project_goals/inherent) - #162171 (Explain LoongArch f16 NaN-boxing in inline asm) - #162173 (fix supposedly unreachable `bug!` being reachable) - #162191 (core: mark float `ClampBounds` methods as `#[inline]`) - #162199 (docs(time): clarify exact seconds for hour and minute)
mgca: Don't ICE when evaluating ValTrees that contain error constants Fixes rust-lang#154632 Fixes rust-lang#150969 `mir::Const::eval` now checks `error_reported()` and returns `Err` instead of converting such values to `ConstValue`.
Rollup of 23 pull requests Successful merges: - #157808 (sanitizers: Implement support for the sanitize ignorelist) - #160564 (volatile: allow accesses to non-AM memory to trap) - #161579 (suggest calling a fn item used as the iterator of a `for` loop) - #162044 (coverage: Resolve spans to file-coordinates in a separate step) - #162120 (Introduce `PerOwnerLoweringState`) - #162151 (Test itanium mangling of `f16` and `f128`) - #162162 (Don't special-case `!` in stability checks anymore) - #162181 (Remove wrong UnusedBraces lint for iterator loop in edition 2024 ) - #162187 (Rename `thir::ExprKind::Use` to `ValueExpr`) - #158401 (mgca: Don't ICE when evaluating ValTrees that contain error constants) - #159873 (fuchsia: Add safestack as a supported sanitizer for x86_64 fuchsia) - #161847 (Preserve visibility in nested macro import suggestions) - #161972 (Improve tests for `#[track_caller]` in async) - #162008 (Render the `box` pattern removal diagnostic more actionable & remove `box` expression recovery) - #162065 (std: don't reference `libc::O_NOFOLLOW` on VxWorks in `set_perm_nofollow`) - #162076 (docs(num): clarify conditions under which error occurs in `impl TryFrom<int> for int`) - #162152 (Revert "retrieve supported GCC targets from the sysroot") - #162153 (Prefer `LLVMGetVersion` for runtime info) - #162168 (fix ICE in project_goals/inherent) - #162171 (Explain LoongArch f16 NaN-boxing in inline asm) - #162173 (fix supposedly unreachable `bug!` being reachable) - #162191 (core: mark float `ClampBounds` methods as `#[inline]`) - #162199 (docs(time): clarify exact seconds for hour and minute)
mgca: Don't ICE when evaluating ValTrees that contain error constants Fixes rust-lang#154632 Fixes rust-lang#150969 `mir::Const::eval` now checks `error_reported()` and returns `Err` instead of converting such values to `ConstValue`.
Rollup of 29 pull requests Successful merges: - #161694 (add `Complex` ABI run-make test) - #162014 (Move more `rustdoc-html` tests using `--test` into the right folder) - #162164 (Revert "Implement Debug for C-like enums with a concatenated string") - #160564 (volatile: allow accesses to non-AM memory to trap) - #161579 (suggest calling a fn item used as the iterator of a `for` loop) - #162044 (coverage: Resolve spans to file-coordinates in a separate step) - #162120 (Introduce `PerOwnerLoweringState`) - #162132 (std: improve safety documentation in UNIX stack overflow code) - #162151 (Test itanium mangling of `f16` and `f128`) - #162162 (Don't special-case `!` in stability checks anymore) - #162181 (Remove wrong UnusedBraces lint for iterator loop in edition 2024 ) - #162187 (Rename `thir::ExprKind::Use` to `ValueExpr`) - #158401 (mgca: Don't ICE when evaluating ValTrees that contain error constants) - #159873 (fuchsia: Add safestack as a supported sanitizer for x86_64 fuchsia) - #161847 (Preserve visibility in nested macro import suggestions) - #161951 (Windows: add fallback if `canonicalize` fails) - #161972 (Improve tests for `#[track_caller]` in async) - #162008 (Render the `box` pattern removal diagnostic more actionable & remove `box` expression recovery) - #162065 (std: don't reference `libc::O_NOFOLLOW` on VxWorks in `set_perm_nofollow`) - #162076 (docs(num): clarify conditions under which error occurs in `impl TryFrom<int> for int`) - #162152 (Revert "retrieve supported GCC targets from the sysroot") - #162153 (Prefer `LLVMGetVersion` for runtime info) - #162168 (fix ICE in project_goals/inherent) - #162171 (Explain LoongArch f16 NaN-boxing in inline asm) - #162173 (fix supposedly unreachable `bug!` being reachable) - #162180 (remove outdated next-solver FIXMEs) - #162191 (core: mark float `ClampBounds` methods as `#[inline]`) - #162195 (docs(time): clarify exact seconds for week and day) - #162199 (docs(time): clarify exact seconds for hour and minute)
…uwer Rollup of 25 pull requests Successful merges: - #161694 (add `Complex` ABI run-make test) - #162014 (Move more `rustdoc-html` tests using `--test` into the right folder) - #162164 (Revert "Implement Debug for C-like enums with a concatenated string") - #160564 (volatile: allow accesses to non-AM memory to trap) - #161579 (suggest calling a fn item used as the iterator of a `for` loop) - #162044 (coverage: Resolve spans to file-coordinates in a separate step) - #162120 (Introduce `PerOwnerLoweringState`) - #162132 (std: improve safety documentation in UNIX stack overflow code) - #162151 (Test itanium mangling of `f16` and `f128`) - #162162 (Don't special-case `!` in stability checks anymore) - #162181 (Remove wrong UnusedBraces lint for iterator loop in edition 2024 ) - #162187 (Rename `thir::ExprKind::Use` to `ValueExpr`) - #158401 (mgca: Don't ICE when evaluating ValTrees that contain error constants) - #159873 (fuchsia: Add safestack as a supported sanitizer for x86_64 fuchsia) - #161135 (Add `f16` and `f128` inline ASM support for PowerPC) - #161847 (Preserve visibility in nested macro import suggestions) - #161972 (Improve tests for `#[track_caller]` in async) - #162008 (Render the `box` pattern removal diagnostic more actionable & remove `box` expression recovery) - #162065 (std: don't reference `libc::O_NOFOLLOW` on VxWorks in `set_perm_nofollow`) - #162076 (docs(num): clarify conditions under which error occurs in `impl TryFrom<int> for int`) - #162111 (Update mailmap for Will Crichton and Petr Hosek) - #162152 (Revert "retrieve supported GCC targets from the sysroot") - #162153 (Prefer `LLVMGetVersion` for runtime info) - #162168 (fix ICE in project_goals/inherent) - #162171 (Explain LoongArch f16 NaN-boxing in inline asm)
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (3b029aa): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: missing data |
…uwer Rollup of 25 pull requests Successful merges: - rust-lang/rust#161694 (add `Complex` ABI run-make test) - rust-lang/rust#162014 (Move more `rustdoc-html` tests using `--test` into the right folder) - rust-lang/rust#162164 (Revert "Implement Debug for C-like enums with a concatenated string") - rust-lang/rust#160564 (volatile: allow accesses to non-AM memory to trap) - rust-lang/rust#161579 (suggest calling a fn item used as the iterator of a `for` loop) - rust-lang/rust#162044 (coverage: Resolve spans to file-coordinates in a separate step) - rust-lang/rust#162120 (Introduce `PerOwnerLoweringState`) - rust-lang/rust#162132 (std: improve safety documentation in UNIX stack overflow code) - rust-lang/rust#162151 (Test itanium mangling of `f16` and `f128`) - rust-lang/rust#162162 (Don't special-case `!` in stability checks anymore) - rust-lang/rust#162181 (Remove wrong UnusedBraces lint for iterator loop in edition 2024 ) - rust-lang/rust#162187 (Rename `thir::ExprKind::Use` to `ValueExpr`) - rust-lang/rust#158401 (mgca: Don't ICE when evaluating ValTrees that contain error constants) - rust-lang/rust#159873 (fuchsia: Add safestack as a supported sanitizer for x86_64 fuchsia) - rust-lang/rust#161135 (Add `f16` and `f128` inline ASM support for PowerPC) - rust-lang/rust#161847 (Preserve visibility in nested macro import suggestions) - rust-lang/rust#161972 (Improve tests for `#[track_caller]` in async) - rust-lang/rust#162008 (Render the `box` pattern removal diagnostic more actionable & remove `box` expression recovery) - rust-lang/rust#162065 (std: don't reference `libc::O_NOFOLLOW` on VxWorks in `set_perm_nofollow`) - rust-lang/rust#162076 (docs(num): clarify conditions under which error occurs in `impl TryFrom<int> for int`) - rust-lang/rust#162111 (Update mailmap for Will Crichton and Petr Hosek) - rust-lang/rust#162152 (Revert "retrieve supported GCC targets from the sysroot") - rust-lang/rust#162153 (Prefer `LLVMGetVersion` for runtime info) - rust-lang/rust#162168 (fix ICE in project_goals/inherent) - rust-lang/rust#162171 (Explain LoongArch f16 NaN-boxing in inline asm)
Fixes #154632
Fixes #150969
mir::Const::evalnow checkserror_reported()and returnsErrinstead of converting such values toConstValue.