-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1082,17 +1082,21 @@ where | |
} | ||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In codegen this isn't even checked, it's a plain |
||
|
||
let (variant_index_relative, op) = if variant_index.as_u32() >= variants_start { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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, | ||
)?; | ||
|
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
There was a problem hiding this comment.
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).