From 8655ad5936dda66a9123251c4c3fa43b7e5b80a4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 5 Dec 2019 14:20:53 +0100 Subject: [PATCH 1/4] codegen: mark invalid SetDiscriminant unreachable --- src/librustc_codegen_ssa/mir/place.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/place.rs b/src/librustc_codegen_ssa/mir/place.rs index e2507394ce681..4b8819d11f4f8 100644 --- a/src/librustc_codegen_ssa/mir/place.rs +++ b/src/librustc_codegen_ssa/mir/place.rs @@ -333,6 +333,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { variant_index: VariantIdx ) { if self.layout.for_variant(bx.cx(), variant_index).abi.is_uninhabited() { + bx.unreachable(); return; } match self.layout.variants { @@ -488,9 +489,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { }, Err(_) => { // This is unreachable as long as runtime - // and compile-time agree on values + // and compile-time agree perfectly. // With floats that won't always be true, - // so we generate an abort. + // so we generate a (safe) abort. bx.abort(); let llval = bx.cx().const_undef( bx.cx().type_ptr_to(bx.cx().backend_type(layout)) From e822235323ba4f732bf2bb930d1a430b70ba5501 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 5 Dec 2019 14:33:37 +0100 Subject: [PATCH 2/4] add a test --- src/test/codegen/set-discriminant-invalid.rs | 43 ++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 src/test/codegen/set-discriminant-invalid.rs diff --git a/src/test/codegen/set-discriminant-invalid.rs b/src/test/codegen/set-discriminant-invalid.rs new file mode 100644 index 0000000000000..65e07fff1e44d --- /dev/null +++ b/src/test/codegen/set-discriminant-invalid.rs @@ -0,0 +1,43 @@ +// compile-flags: -C opt-level=0 +#![crate_type = "lib"] + +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 for Api +{ + type Source = ApiError; + // CHECK-LABEL: @into_error + // CHECK: unreachable + // Also check the next two instructions to make sure we do not match against `unreachable` + // elsewhere in the code (e.g., in the closure bode). + // CHECK-NEXT: load + // CHECK-NEXT: ret + #[no_mangle] + fn into_error(self, error: Self::Source) -> Error { + Error::Api { + source: (|v| v)(error), + } + } +} + +pub trait IntoError +{ + /// The underlying error + type Source; + + /// Combine the information to produce the error + fn into_error(self, source: Self::Source) -> E; +} From e5d50e3e88f88e8a8b20e90bb0d561e1ed71fe5f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 5 Dec 2019 14:38:24 +0100 Subject: [PATCH 3/4] comments --- src/librustc_codegen_ssa/mir/operand.rs | 5 +++-- src/librustc_codegen_ssa/mir/place.rs | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/operand.rs b/src/librustc_codegen_ssa/mir/operand.rs index 310b8aeb4db09..a6dec81274915 100644 --- a/src/librustc_codegen_ssa/mir/operand.rs +++ b/src/librustc_codegen_ssa/mir/operand.rs @@ -475,9 +475,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { }, } // Allow RalfJ to sleep soundly knowing that even refactorings that remove - // the above error (or silence it under some conditions) will not cause UB + // the above error (or silence it under some conditions) will not cause UB. bx.abort(); - // We've errored, so we don't have to produce working code. + // We still have to return an operand but it doesn't matter, + // this code is unreachable. let ty = self.monomorphize(&constant.literal.ty); let layout = bx.cx().layout_of(ty); bx.load_operand(PlaceRef::new_sized( diff --git a/src/librustc_codegen_ssa/mir/place.rs b/src/librustc_codegen_ssa/mir/place.rs index 4b8819d11f4f8..b8ff2249dcb04 100644 --- a/src/librustc_codegen_ssa/mir/place.rs +++ b/src/librustc_codegen_ssa/mir/place.rs @@ -493,6 +493,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // With floats that won't always be true, // so we generate a (safe) abort. bx.abort(); + // We still have to return a place but it doesn't matter, + // this code is unreachable. let llval = bx.cx().const_undef( bx.cx().type_ptr_to(bx.cx().backend_type(layout)) ); From f5bd94768a5ca901eb7555a4c3fd4d3005c1ad76 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 5 Dec 2019 23:59:30 +0100 Subject: [PATCH 4/4] use abort instead of unreachable --- src/librustc_codegen_ssa/mir/block.rs | 6 ++++++ src/librustc_codegen_ssa/mir/place.rs | 4 +++- src/test/codegen/set-discriminant-invalid.rs | 6 +++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 6dccf329c9f64..ce703f2433506 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -261,7 +261,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { if self.fn_abi.ret.layout.abi.is_uninhabited() { // Functions with uninhabited return values are marked `noreturn`, // so we should make sure that we never actually do. + // We play it safe by using a well-defined `abort`, but we could go for immediate UB + // if that turns out to be helpful. bx.abort(); + // `abort` does not terminate the block, so we still need to generate + // an `unreachable` terminator after it. bx.unreachable(); return; } @@ -825,6 +829,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { mir::TerminatorKind::Abort => { bx.abort(); + // `abort` does not terminate the block, so we still need to generate + // an `unreachable` terminator after it. bx.unreachable(); } diff --git a/src/librustc_codegen_ssa/mir/place.rs b/src/librustc_codegen_ssa/mir/place.rs index b8ff2249dcb04..e60b8861faf85 100644 --- a/src/librustc_codegen_ssa/mir/place.rs +++ b/src/librustc_codegen_ssa/mir/place.rs @@ -333,7 +333,9 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { variant_index: VariantIdx ) { if self.layout.for_variant(bx.cx(), variant_index).abi.is_uninhabited() { - bx.unreachable(); + // We play it safe by using a well-defined `abort`, but we could go for immediate UB + // if that turns out to be helpful. + bx.abort(); return; } match self.layout.variants { diff --git a/src/test/codegen/set-discriminant-invalid.rs b/src/test/codegen/set-discriminant-invalid.rs index 65e07fff1e44d..d9614f062b7e9 100644 --- a/src/test/codegen/set-discriminant-invalid.rs +++ b/src/test/codegen/set-discriminant-invalid.rs @@ -20,9 +20,9 @@ impl IntoError for Api { type Source = ApiError; // CHECK-LABEL: @into_error - // CHECK: unreachable - // Also check the next two instructions to make sure we do not match against `unreachable` - // elsewhere in the code (e.g., in the closure bode). + // CHECK: llvm.trap() + // Also check the next two instructions to make sure we do not match against `trap` + // elsewhere in the code. // CHECK-NEXT: load // CHECK-NEXT: ret #[no_mangle]