Do not assert in op_to_const. #117441

cjgillot · 2023-10-31T11:33:23Z

op_to_const is used in try_destructure_mir_constant_for_diagnostics, which may encounter invalid constants created by optimizations and debugging.

r? @oli-obk

Fixes #117368

oli-obk · 2023-10-31T11:40:43Z

Hmm... While you're at it, please also rename the hook to _for_user_output and adjust the documentation to explain that it is also used for mir dumps.

Can you write a mir-opt test demonstrating what is being rendered instead of the ui test? Or does it not actually end up showing up?

rustbot · 2023-10-31T17:41:56Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cjgillot · 2023-10-31T18:43:20Z

tests/mir-opt/pre-codegen/issue_117368_print_invalid_constant.main.GVN.32bit.panic-abort.diff

+
+      bb3: {
+-         _1 = move ((_2 as Some).0: std::alloc::Layout);
+         _1 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): ptr::alignment::AlignmentEnum32) }};


@oli-obk this is what we were ICEing on. This is a fancy way of having an uninit byte range in a MIR constant. This bb is unreachable, so this does not matter if we output such degenerate constant.

oli-obk · 2023-10-31T19:41:10Z

@bors r+ rollup

Thanks! This helps me a lot

bors · 2023-10-31T19:41:12Z

📌 Commit 6ca7bfd has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-11-01T02:06:19Z

☔ The latest upstream changes (presumably #117459) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2023-11-01T11:59:52Z

@bors r+

bors · 2023-11-01T11:59:55Z

📌 Commit ef196ae has been approved by oli-obk

It is now in the queue for this repository.

Do not assert in op_to_const. `op_to_const` is used in `try_destructure_mir_constant_for_diagnostics`, which may encounter invalid constants created by optimizations and debugging. r? `@oli-obk` Fixes rust-lang#117368

…llaumeGomez Rollup of 3 pull requests Successful merges: - rust-lang#117373 (Avoid the path trimming ICE lint in error reporting) - rust-lang#117441 (Do not assert in op_to_const.) - rust-lang#117488 (Update minifier-rs version to 0.3.0) r? `@ghost` `@rustbot` modify labels: rollup

compiler-errors · 2023-11-01T16:44:22Z

@bors r-

This test failed in a rollup (#117490 (comment))

@GuillaumeGomez: This is the PR that failed, not #117373.

GuillaumeGomez · 2023-11-01T16:46:25Z

Ah I see. Thanks for the clarification. :)

cjgillot · 2023-11-01T17:29:57Z

@bors r=oli-obk

bors · 2023-11-01T17:29:59Z

📌 Commit 0f8f77f has been approved by oli-obk

It is now in the queue for this repository.

RalfJung · 2023-11-01T17:31:23Z

op_to_const is used in try_destructure_mir_constant_for_diagnostics, which may encounter invalid constants created by optimizations and debugging.

Maybe it shouldn't? I don't like this kind of silent fallback. We should never "catch" interpreter errors on any code path that is also used for regular consts.

@bors r-

RalfJung · 2023-11-01T17:31:55Z

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -132,8 +132,8 @@ pub(super) fn op_to_const<'tcx>(
        // functionality.)
        _ => false,
    };
-    let immediate = if force_as_immediate {
-        Right(ecx.read_immediate(op).expect("normalization works on validated constants"))
+    let immediate = if force_as_immediate && let Ok(imm) = ecx.read_immediate(op) {


This is "catching" an interpreter error, and this codepath is used for regular consts, not just for diagnostics. That's fragile, I don't think we should do this.

IIUC, this code path is mostly an optimization. Nothing would break if we were to set force_as_immediate as false: we do not need to represent scalars as a ConstValue::Scalar, it's just more efficient.

That's wrong; many parts of the compiler rely on try_to_scalar to work for integer types, and will ICE if we don't use the "immediate" code path here.

There's a comment just a few lines above explaining that:

// All scalar types should be stored as `ConstValue::Scalar`. This is needed to make // `ConstValue::try_to_scalar` efficient; we want that to work for *all* constants of scalar // type (it's used throughout the compiler and having it work just on literals is not enough) // and we want it to be fast (i.e., don't go to an `Allocation` and reconstruct the `Scalar` // from its byte-serialized form).

Please suggest how we could word that more clearly to avoid such misunderstandings in the future.

RalfJung · 2023-11-01T18:40:55Z

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -132,8 +133,14 @@ pub(super) fn op_to_const<'tcx>(
        // functionality.)
        _ => false,
    };
-    let immediate = if force_as_immediate && let Ok(imm) = ecx.read_immediate(op) {
-        Right(imm)
+    let immediate = if force_as_immediate {


I wonder, would it work to just say if !force_as_immediate && force_as_immediate here? Then we could avoid "catching" entirely.

I tried it. This would degrade diagnostics significantly, as we wont be able to output an Option<u32> as a Some(99_usize), just Some(Indirect { stuff }), which IMO is much harder to use.

This could be alleviated by equipping the diagnostic code with support for printing integers that are Indirect. Not sure if that's worth it though.

RalfJung · 2023-11-01T18:55:29Z

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -110,6 +110,7 @@ pub(crate) fn mk_eval_cx<'mir, 'tcx>(
 pub(super) fn op_to_const<'tcx>(


Please extend the doc comment (sadly github doesn't let me put a suggestion there...)

/// The `for_diagnostics` flag turns the usual rules for returning `ConstValue::Scalar` into a best-effort /// attempt. This is not okay for use in const-eval sine it breaks invariants rustc relies on, but /// it is okay for diagnostics which will just give up gracefully when they encounter an `Indirect` they /// cannot handle.

RalfJung · 2023-11-01T19:09:37Z

Thanks!

@bors r=oli-obk,RalfJung

bors · 2023-11-01T19:09:39Z

📌 Commit f512f91 has been approved by oli-obk,RalfJung

It is now in the queue for this repository.

…RalfJung Do not assert in op_to_const. `op_to_const` is used in `try_destructure_mir_constant_for_diagnostics`, which may encounter invalid constants created by optimizations and debugging. r? `@oli-obk` Fixes rust-lang#117368

…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#117298 (Recover from missing param list in function definitions) - rust-lang#117373 (Avoid the path trimming ICE lint in error reporting) - rust-lang#117441 (Do not assert in op_to_const.) - rust-lang#117488 (Update minifier-rs version to 0.3.0) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117441 - cjgillot:diag-noassert, r=oli-obk,RalfJung Do not assert in op_to_const. `op_to_const` is used in `try_destructure_mir_constant_for_diagnostics`, which may encounter invalid constants created by optimizations and debugging. r? ``@oli-obk`` Fixes rust-lang#117368

rustbot assigned oli-obk Oct 31, 2023

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 Oct 31, 2023

cjgillot force-pushed the diag-noassert branch from d6c1bb5 to f66f6c9 Compare October 31, 2023 11:35

cjgillot commented Oct 31, 2023

View reviewed changes

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 Oct 31, 2023

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2023

cjgillot force-pushed the diag-noassert branch from 6ca7bfd to ef196ae Compare November 1, 2023 11:11

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2023

GuillaumeGomez mentioned this pull request Nov 1, 2023

Rollup of 3 pull requests #117490

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2023

cjgillot added 4 commits November 1, 2023 16:49

Do not assert in op_to_const.

c2f49e9

Make ui into mir-opt test.

a2e151c

Rename hook.

3a55c28

Rebase fallout.

0f8f77f

cjgillot force-pushed the diag-noassert branch from ef196ae to 0f8f77f Compare November 1, 2023 16:55

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2023

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2023

RalfJung reviewed Nov 1, 2023

View reviewed changes

RalfJung mentioned this pull request Nov 1, 2023

Avoid the path trimming ICE lint in error reporting #117373

Merged

Specify diagnostic path.

224e290

RalfJung reviewed Nov 1, 2023

View reviewed changes

Comment for_diagnostics flag.

f512f91

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2023

This was referenced Nov 1, 2023

Rollup of 4 pull requests #117497

Closed

Rollup of 4 pull requests #117498

Merged

bors merged commit 4e437be into rust-lang:master Nov 2, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Nov 2, 2023

github-actions bot mentioned this pull request Nov 2, 2023

Automated pull from upstream master ferrocene/ferrocene#80

Merged

cjgillot deleted the diag-noassert branch November 2, 2023 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not assert in op_to_const. #117441

Do not assert in op_to_const. #117441

cjgillot commented Oct 31, 2023

oli-obk commented Oct 31, 2023

rustbot commented Oct 31, 2023

cjgillot Oct 31, 2023

oli-obk commented Oct 31, 2023

bors commented Oct 31, 2023

bors commented Nov 1, 2023

oli-obk commented Nov 1, 2023

bors commented Nov 1, 2023

compiler-errors commented Nov 1, 2023

GuillaumeGomez commented Nov 1, 2023

cjgillot commented Nov 1, 2023

bors commented Nov 1, 2023

RalfJung commented Nov 1, 2023

RalfJung Nov 1, 2023

cjgillot Nov 1, 2023

RalfJung Nov 1, 2023 •

edited

Loading

RalfJung Nov 1, 2023

RalfJung Nov 1, 2023

cjgillot Nov 1, 2023

RalfJung Nov 1, 2023 •

edited

Loading

RalfJung Nov 1, 2023

RalfJung commented Nov 1, 2023

bors commented Nov 1, 2023

		@@ -110,6 +110,7 @@ pub(crate) fn mk_eval_cx<'mir, 'tcx>(
		pub(super) fn op_to_const<'tcx>(

Do not assert in op_to_const. #117441

Do not assert in op_to_const. #117441

Conversation

cjgillot commented Oct 31, 2023

oli-obk commented Oct 31, 2023

rustbot commented Oct 31, 2023

cjgillot Oct 31, 2023

Choose a reason for hiding this comment

oli-obk commented Oct 31, 2023

bors commented Oct 31, 2023

bors commented Nov 1, 2023

oli-obk commented Nov 1, 2023

bors commented Nov 1, 2023

compiler-errors commented Nov 1, 2023

GuillaumeGomez commented Nov 1, 2023

cjgillot commented Nov 1, 2023

bors commented Nov 1, 2023

RalfJung commented Nov 1, 2023

RalfJung Nov 1, 2023

Choose a reason for hiding this comment

cjgillot Nov 1, 2023

Choose a reason for hiding this comment

RalfJung Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 1, 2023

Choose a reason for hiding this comment

RalfJung Nov 1, 2023

Choose a reason for hiding this comment

cjgillot Nov 1, 2023

Choose a reason for hiding this comment

RalfJung Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 1, 2023

Choose a reason for hiding this comment

RalfJung commented Nov 1, 2023

bors commented Nov 1, 2023

RalfJung Nov 1, 2023 •

edited

Loading

RalfJung Nov 1, 2023 •

edited

Loading