From 6a1c887794252bef8e7fe0f4ea002633ac846b5d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 12 Sep 2023 14:44:38 +0200 Subject: [PATCH] fix clippy (and MIR printing) handling of ConstValue::Indirect slices --- .../rustc_middle/src/mir/interpret/value.rs | 53 ++++++++++++++++++- compiler/rustc_middle/src/mir/mod.rs | 35 ++++-------- src/tools/clippy/clippy_utils/src/consts.rs | 17 ++---- ...ble_variable_aggregate.main.ConstProp.diff | 2 +- ...ropagation.main.ConstProp.panic-abort.diff | 4 +- ...opagation.main.ConstProp.panic-unwind.diff | 4 +- 6 files changed, 71 insertions(+), 44 deletions(-) diff --git a/compiler/rustc_middle/src/mir/interpret/value.rs b/compiler/rustc_middle/src/mir/interpret/value.rs index 6207a35e4f315..c169733ad742c 100644 --- a/compiler/rustc_middle/src/mir/interpret/value.rs +++ b/compiler/rustc_middle/src/mir/interpret/value.rs @@ -9,7 +9,10 @@ use rustc_apfloat::{ use rustc_macros::HashStable; use rustc_target::abi::{HasDataLayout, Size}; -use crate::ty::{ParamEnv, ScalarInt, Ty, TyCtxt}; +use crate::{ + mir::interpret::alloc_range, + ty::{ParamEnv, ScalarInt, Ty, TyCtxt}, +}; use super::{ AllocId, ConstAllocation, InterpResult, Pointer, PointerArithmetic, Provenance, @@ -114,6 +117,54 @@ impl<'tcx> ConstValue<'tcx> { pub fn from_target_usize(i: u64, cx: &impl HasDataLayout) -> Self { ConstValue::Scalar(Scalar::from_target_usize(i, cx)) } + + /// Must only be called on constants of type `&str` or `&[u8]`! + pub fn try_get_slice_bytes_for_diagnostics(&self, tcx: TyCtxt<'tcx>) -> Option<&'tcx [u8]> { + let (data, start, end) = match self { + ConstValue::Scalar(_) | ConstValue::ZeroSized => { + bug!("`try_get_slice_bytes` on non-slice constant") + } + &ConstValue::Slice { data, start, end } => (data, start, end), + &ConstValue::Indirect { alloc_id, offset } => { + // The reference itself is stored behind an indirection. + // Load the reference, and then load the actual slice contents. + let a = tcx.global_alloc(alloc_id).unwrap_memory().inner(); + let ptr_size = tcx.data_layout.pointer_size; + if a.size() < offset + 2 * ptr_size { + // (partially) dangling reference + return None; + } + // Read the wide pointer components. + let ptr = a + .read_scalar( + &tcx, + alloc_range(offset, ptr_size), + /* read_provenance */ true, + ) + .ok()?; + let ptr = ptr.to_pointer(&tcx).ok()?; + let len = a + .read_scalar( + &tcx, + alloc_range(offset + ptr_size, ptr_size), + /* read_provenance */ false, + ) + .ok()?; + let len = len.to_target_usize(&tcx).ok()?; + let len: usize = len.try_into().ok()?; + if len == 0 { + return Some(&[]); + } + // Non-empty slice, must have memory. We know this is a relative pointer. + let (inner_alloc_id, offset) = ptr.into_parts(); + let data = tcx.global_alloc(inner_alloc_id?).unwrap_memory(); + (data, offset.bytes_usize(), offset.bytes_usize() + len) + } + }; + + // This is for diagnostics only, so we are okay to use `inspect_with_uninit_and_ptr_outside_interpreter`. + Some(data.inner().inspect_with_uninit_and_ptr_outside_interpreter(start..end)) + } } /// A `Scalar` represents an immediate, primitive value existing outside of a diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 6e3409f17ca0c..56b72d0b23599 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -2887,31 +2887,16 @@ fn pretty_print_const_value<'tcx>( let u8_type = tcx.types.u8; match (ct, ty.kind()) { // Byte/string slices, printed as (byte) string literals. - (ConstValue::Slice { data, start, end }, ty::Ref(_, inner, _)) => { - match inner.kind() { - ty::Slice(t) => { - if *t == u8_type { - // The `inspect` here is okay since we checked the bounds, and `u8` carries - // no provenance (we have an active slice reference here). We don't use - // this result to affect interpreter execution. - let byte_str = data - .inner() - .inspect_with_uninit_and_ptr_outside_interpreter(start..end); - pretty_print_byte_str(fmt, byte_str)?; - return Ok(()); - } - } - ty::Str => { - // The `inspect` here is okay since we checked the bounds, and `str` carries - // no provenance (we have an active `str` reference here). We don't use this - // result to affect interpreter execution. - let slice = data - .inner() - .inspect_with_uninit_and_ptr_outside_interpreter(start..end); - fmt.write_str(&format!("{:?}", String::from_utf8_lossy(slice)))?; - return Ok(()); - } - _ => {} + (_, ty::Ref(_, inner_ty, _)) if matches!(inner_ty.kind(), ty::Str) => { + if let Some(data) = ct.try_get_slice_bytes_for_diagnostics(tcx) { + fmt.write_str(&format!("{:?}", String::from_utf8_lossy(data)))?; + return Ok(()); + } + } + (_, ty::Ref(_, inner_ty, _)) if matches!(inner_ty.kind(), ty::Slice(t) if *t == u8_type) => { + if let Some(data) = ct.try_get_slice_bytes_for_diagnostics(tcx) { + pretty_print_byte_str(fmt, data)?; + return Ok(()); } } (ConstValue::Indirect { alloc_id, offset }, ty::Array(t, n)) if *t == u8_type => { diff --git a/src/tools/clippy/clippy_utils/src/consts.rs b/src/tools/clippy/clippy_utils/src/consts.rs index 61b944667a4ae..fcb90c63a6fc6 100644 --- a/src/tools/clippy/clippy_utils/src/consts.rs +++ b/src/tools/clippy/clippy_utils/src/consts.rs @@ -671,19 +671,10 @@ pub fn miri_to_const<'tcx>(lcx: &LateContext<'tcx>, result: mir::ConstantKind<'t ty::RawPtr(_) => Some(Constant::RawPtr(int.assert_bits(int.size()))), _ => None, }, - mir::ConstantKind::Val(ConstValue::Slice { data, start, end }, _) => match result.ty().kind() { - ty::Ref(_, tam, _) => match tam.kind() { - ty::Str => String::from_utf8( - data.inner() - .inspect_with_uninit_and_ptr_outside_interpreter(start..end) - .to_owned(), - ) - .ok() - .map(Constant::Str), - _ => None, - }, - _ => None, - }, + mir::ConstantKind::Val(cv, _) if matches!(result.ty().kind(), ty::Ref(_, inner_ty, _) if matches!(inner_ty.kind(), ty::Str)) => { + let data = cv.try_get_slice_bytes_for_diagnostics(lcx.tcx)?; + String::from_utf8(data.to_owned()).ok().map(Constant::Str) + } mir::ConstantKind::Val(ConstValue::Indirect { alloc_id, offset: _ }, _) => { let alloc = lcx.tcx.global_alloc(alloc_id).unwrap_memory(); match result.ty().kind() { diff --git a/tests/mir-opt/const_prop/mutable_variable_aggregate.main.ConstProp.diff b/tests/mir-opt/const_prop/mutable_variable_aggregate.main.ConstProp.diff index 3f3135ff4fe4b..56a127ae31ebe 100644 --- a/tests/mir-opt/const_prop/mutable_variable_aggregate.main.ConstProp.diff +++ b/tests/mir-opt/const_prop/mutable_variable_aggregate.main.ConstProp.diff @@ -27,7 +27,7 @@ } + } + -+ alloc6 (size: 8, align: 4) { ++ alloc7 (size: 8, align: 4) { + 2a 00 00 00 63 00 00 00 │ *...c... + } + diff --git a/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-abort.diff b/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-abort.diff index 6e676d3a0e2ef..988ef7dd2254a 100644 --- a/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-abort.diff +++ b/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-abort.diff @@ -31,11 +31,11 @@ } + } + -+ alloc8 (size: 8, align: 4) { ++ alloc9 (size: 8, align: 4) { + 01 00 00 00 02 00 00 00 │ ........ + } + -+ alloc7 (size: 8, align: 4) { ++ alloc8 (size: 8, align: 4) { + 01 00 00 00 02 00 00 00 │ ........ + } + diff --git a/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-unwind.diff b/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-unwind.diff index d3db3ad066b90..298446197203d 100644 --- a/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-unwind.diff +++ b/tests/mir-opt/const_prop/tuple_literal_propagation.main.ConstProp.panic-unwind.diff @@ -31,11 +31,11 @@ } + } + -+ alloc8 (size: 8, align: 4) { ++ alloc9 (size: 8, align: 4) { + 01 00 00 00 02 00 00 00 │ ........ + } + -+ alloc7 (size: 8, align: 4) { ++ alloc8 (size: 8, align: 4) { + 01 00 00 00 02 00 00 00 │ ........ + } +