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

[const-prop] Fix ICE due to underflow when calculating enum discriminant #66857

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/librustc_mir/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,17 +1082,21 @@ where
}
Copy link
Member

Choose a reason for hiding this comment

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

Oh, btw, this check seems pointless, it's impossible for variant_index to be out of range (without rustc being broken), so you could just ICE here if you wanted to (but there's not really any point, dest.layout.for_variant(variant_index) would panic trying to index with it, anyway).

if variant_index != dataful_variant {
let variants_start = niche_variants.start().as_u32();
let variant_index_relative = variant_index.as_u32()
.checked_sub(variants_start)
.expect("overflow computing relative variant idx");
Copy link
Member

Choose a reason for hiding this comment

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

In codegen this isn't even checked, it's a plain -, because this is impossible for indices.


let (variant_index_relative, op) = if variant_index.as_u32() >= variants_start {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use overflowing_sub here to remove some duplication. The add/sub decision can be made on the overflow bool returned by overflowing_sub

(variant_index.as_u32() - variants_start, mir::BinOp::Add)
} else {
(variants_start - variant_index.as_u32(), mir::BinOp::Sub)
};

// We need to use machine arithmetic when taking into account `niche_start`:
// discr_val = variant_index_relative + niche_start_val
let discr_layout = self.layout_of(discr_layout.value.to_int_ty(*self.tcx))?;
let niche_start_val = ImmTy::from_uint(niche_start, discr_layout);
let variant_index_relative_val =
ImmTy::from_uint(variant_index_relative, discr_layout);
let discr_val = self.binary_op(
mir::BinOp::Add,
op,
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right to me... in the 2nd branch above, what happens (compared to earlier) is that variant_index_relative is negative, but here by changing the operator you are negating the second operand, niche_start_val. So the entire result is the negation of what it used to be. Don't you also have to swap the operands?

Either way, this needs a detailed comment why both branches above are correct. Also we should understand why LLVM codegen doesn't need a branch like this.

variant_index_relative_val,
niche_start_val,
)?;
Expand Down
39 changes: 39 additions & 0 deletions src/test/ui/consts/issue-66787.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// build-pass
// compile-flags: --crate-type lib

// Regression test for ICE which occurred when const propagating an enum whose discriminant
// niche triggered an integer underflow conmupting a delta.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to also trigger this ICE in standalone Miri? I'd prefer to have such important parts of the Miri engine not just covered indirectly through an optimization.

Copy link
Member

Choose a reason for hiding this comment

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

Given #66857 (comment) I'm not sure that's possible, since this is necessarily an initialization of an uninhabited variant, and that's dead code.

Miri likely wouldn't have had this bug for so long if it was actually executing the code that is being constant-folded here.


pub enum ApiError {}
#[allow(dead_code)]
pub struct TokioError {
b: bool,
}
pub enum Error {
Api {
source: ApiError,
},
Ethereum,
Tokio {
source: TokioError,
},
}
struct Api;
impl IntoError<Error> for Api
{
type Source = ApiError;
fn into_error(self, error: Self::Source) -> Error {
Error::Api {
source: (|v| v)(error),
}
}
}

pub trait IntoError<E>
{
/// The underlying error
type Source;

/// Combine the information to produce the error
fn into_error(self, source: Self::Source) -> E;
}