Skip to content

We don't need NonNull::as_ptr debuginfo #133899

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

Merged
merged 1 commit into from
Dec 13, 2024
Merged
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
3 changes: 3 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ declare_passes! {
mod simplify_comparison_integral : SimplifyComparisonIntegral;
mod single_use_consts : SingleUseConsts;
mod sroa : ScalarReplacementOfAggregates;
mod strip_debuginfo : StripDebugInfo;
mod unreachable_enum_branching : UnreachableEnumBranching;
mod unreachable_prop : UnreachablePropagation;
mod validate : Validator;
Expand Down Expand Up @@ -699,6 +700,8 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
&o1(simplify_branches::SimplifyConstCondition::Final),
&o1(remove_noop_landing_pads::RemoveNoopLandingPads),
&o1(simplify::SimplifyCfg::Final),
// After the last SimplifyCfg, because this wants one-block functions.
&strip_debuginfo::StripDebugInfo,
&copy_prop::CopyProp,
&dead_store_elimination::DeadStoreElimination::Final,
&nrvo::RenameReturnPlace,
Expand Down
34 changes: 34 additions & 0 deletions compiler/rustc_mir_transform/src/strip_debuginfo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use rustc_session::config::MirStripDebugInfo;

/// Conditionally remove some of the VarDebugInfo in MIR.
///
/// In particular, stripping non-parameter debug info for tiny, primitive-like
/// methods in core saves work later, and nobody ever wanted to use it anyway.
pub(super) struct StripDebugInfo;

impl<'tcx> crate::MirPass<'tcx> for StripDebugInfo {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
sess.opts.unstable_opts.mir_strip_debuginfo != MirStripDebugInfo::None
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
match tcx.sess.opts.unstable_opts.mir_strip_debuginfo {
MirStripDebugInfo::None => return,
MirStripDebugInfo::AllLocals => {}
MirStripDebugInfo::LocalsInTinyFunctions
if let TerminatorKind::Return { .. } =
body.basic_blocks[START_BLOCK].terminator().kind => {}
MirStripDebugInfo::LocalsInTinyFunctions => return,
}

body.var_debug_info.retain(|vdi| {
matches!(
vdi.value,
VarDebugInfoContents::Place(place)
if place.local.as_usize() <= body.arg_count && place.local != RETURN_PLACE,
)
});
}
}
16 changes: 12 additions & 4 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,13 @@ impl ToString for DebugInfoCompression {
}
}

#[derive(Clone, Copy, Debug, PartialEq, Hash)]
pub enum MirStripDebugInfo {
None,
LocalsInTinyFunctions,
AllLocals,
}

/// Split debug-information is enabled by `-C split-debuginfo`, this enum is only used if split
/// debug-information is enabled (in either `Packed` or `Unpacked` modes), and the platform
/// uses DWARF for debug-information.
Expand Down Expand Up @@ -2900,10 +2907,10 @@ pub(crate) mod dep_tracking {
BranchProtection, CFGuard, CFProtection, CollapseMacroDebuginfo, CoverageOptions,
CrateType, DebugInfo, DebugInfoCompression, ErrorOutputType, FmtDebug, FunctionReturn,
InliningThreshold, InstrumentCoverage, InstrumentXRay, LinkerPluginLto, LocationDetail,
LtoCli, NextSolverConfig, OomStrategy, OptLevel, OutFileName, OutputType, OutputTypes,
PatchableFunctionEntry, Polonius, RemapPathScopeComponents, ResolveDocLinks,
SourceFileHashAlgorithm, SplitDwarfKind, SwitchWithOptPath, SymbolManglingVersion,
WasiExecModel,
LtoCli, MirStripDebugInfo, NextSolverConfig, OomStrategy, OptLevel, OutFileName,
OutputType, OutputTypes, PatchableFunctionEntry, Polonius, RemapPathScopeComponents,
ResolveDocLinks, SourceFileHashAlgorithm, SplitDwarfKind, SwitchWithOptPath,
SymbolManglingVersion, WasiExecModel,
};
use crate::lint;
use crate::utils::NativeLib;
Expand Down Expand Up @@ -2971,6 +2978,7 @@ pub(crate) mod dep_tracking {
LtoCli,
DebugInfo,
DebugInfoCompression,
MirStripDebugInfo,
CollapseMacroDebuginfo,
UnstableFeatures,
NativeLib,
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,8 @@ mod desc {
pub(crate) const parse_cfprotection: &str = "`none`|`no`|`n` (default), `branch`, `return`, or `full`|`yes`|`y` (equivalent to `branch` and `return`)";
pub(crate) const parse_debuginfo: &str = "either an integer (0, 1, 2), `none`, `line-directives-only`, `line-tables-only`, `limited`, or `full`";
pub(crate) const parse_debuginfo_compression: &str = "one of `none`, `zlib`, or `zstd`";
pub(crate) const parse_mir_strip_debuginfo: &str =
"one of `none`, `locals-in-tiny-functions`, or `all-locals`";
pub(crate) const parse_collapse_macro_debuginfo: &str = "one of `no`, `external`, or `yes`";
pub(crate) const parse_strip: &str = "either `none`, `debuginfo`, or `symbols`";
pub(crate) const parse_linker_flavor: &str = ::rustc_target::spec::LinkerFlavorCli::one_of();
Expand Down Expand Up @@ -925,6 +927,16 @@ pub mod parse {
true
}

pub(crate) fn parse_mir_strip_debuginfo(slot: &mut MirStripDebugInfo, v: Option<&str>) -> bool {
match v {
Some("none") => *slot = MirStripDebugInfo::None,
Some("locals-in-tiny-functions") => *slot = MirStripDebugInfo::LocalsInTinyFunctions,
Some("all-locals") => *slot = MirStripDebugInfo::AllLocals,
_ => return false,
};
true
}

pub(crate) fn parse_linker_flavor(slot: &mut Option<LinkerFlavorCli>, v: Option<&str>) -> bool {
match v.and_then(LinkerFlavorCli::from_str) {
Some(lf) => *slot = Some(lf),
Expand Down Expand Up @@ -1893,6 +1905,8 @@ options! {
#[rustc_lint_opt_deny_field_access("use `Session::mir_opt_level` instead of this field")]
mir_opt_level: Option<usize> = (None, parse_opt_number, [TRACKED],
"MIR optimization level (0-4; default: 1 in non optimized builds and 2 in optimized builds)"),
mir_strip_debuginfo: MirStripDebugInfo = (MirStripDebugInfo::None, parse_mir_strip_debuginfo, [TRACKED],
"Whether to remove some of the MIR debug info from methods. Default: None"),
move_size_limit: Option<usize> = (None, parse_opt_number, [TRACKED],
"the size at which the `large_assignments` lint starts to be emitted"),
mutable_noalias: bool = (true, parse_bool, [TRACKED],
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/src/core/builder/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,11 @@ impl Builder<'_> {
// even if we're not going to output debuginfo for the crate we're currently building,
// so that it'll be available when downstream consumers of std try to use it.
rustflags.arg("-Zinline-mir-preserve-debug");

// FIXME: always pass this after the next `#[cfg(bootstrap)]` update.
if compiler.stage != 0 {
rustflags.arg("-Zmir_strip_debuginfo=locals-in-tiny-functions");
}
}

Cargo {
Expand Down
5 changes: 3 additions & 2 deletions tests/codegen/mem-replace-big-type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ pub fn replace_big(dst: &mut Big, src: Big) -> Big {
// CHECK-NOT: call void @llvm.memcpy

// For a large type, we expect exactly three `memcpy`s
// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}sret([56 x i8])
// CHECK-LABEL: define internal void @{{.+}}mem{{.+}}replace{{.+}}(ptr
// CHECK-SAME: sret([56 x i8]){{.+}}[[RESULT:%.+]], ptr{{.+}}%dest, ptr{{.+}}%src)
// CHECK-NOT: call void @llvm.memcpy
// CHECK: call void @llvm.memcpy.{{.+}}(ptr align 8 %result, ptr align 8 %dest, i{{.*}} 56, i1 false)
// CHECK: call void @llvm.memcpy.{{.+}}(ptr align 8 [[RESULT]], ptr align 8 %dest, i{{.*}} 56, i1 false)
// CHECK-NOT: call void @llvm.memcpy
// CHECK: call void @llvm.memcpy.{{.+}}(ptr align 8 %dest, ptr align 8 %src, i{{.*}} 56, i1 false)
// CHECK-NOT: call void @llvm.memcpy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
+ scope 3 (inlined Pin::<&mut {coroutine@$DIR/inline_coroutine.rs:20:5: 20:8}>::new) {
+ debug pointer => _3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this one also be gone? Pin::new (after inlining the function call in its body and stripping debug info) is just the same as Pin::new_unchecked, but its debug info seems to stay around.

Copy link
Member Author

Choose a reason for hiding this comment

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

pointer is the parameter to Pin::new, so it's not removed

pub const fn new(pointer: Ptr) -> Pin<Ptr> {

(Rather than if it were a local in the function)

Copy link
Contributor

Choose a reason for hiding this comment

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

But isn't the same true for the pointer arg of new_unchecked?

Ah, if the currently-being-optimized function were in libcore, the arg of new would also be gone...

Makes sense, but subtle

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, definitely subtle. We could be less subtle by just removing all the debuginfo from trivial functions, but I thought it would be worth allowing at least that outer bit so that if you're unsure what you're calling into core with, that first step in having those arguments visible seemed potentially useful.

Basically I'm trying to make as small a change as I can while still avoiding the bad incentive of local variables in core making the compiler slower.

+ scope 4 (inlined Pin::<&mut {coroutine@$DIR/inline_coroutine.rs:20:5: 20:8}>::new_unchecked) {
+ debug pointer => _3;
+ }
+ }
+ scope 5 (inlined g::{closure#0}) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
+ scope 3 (inlined Pin::<&mut {coroutine@$DIR/inline_coroutine.rs:20:5: 20:8}>::new) {
+ debug pointer => _3;
+ scope 4 (inlined Pin::<&mut {coroutine@$DIR/inline_coroutine.rs:20:5: 20:8}>::new_unchecked) {
+ debug pointer => _3;
+ }
+ }
+ scope 5 (inlined g::{closure#0}) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,48 +7,36 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
debug self => _1;
scope 2 (inlined Vec::<u8>::as_slice) {
debug self => _1;
let mut _9: *const u8;
let mut _10: usize;
let mut _7: *const u8;
let mut _8: usize;
scope 3 (inlined Vec::<u8>::as_ptr) {
debug self => _1;
let mut _2: &alloc::raw_vec::RawVec<u8>;
let mut _8: *mut u8;
let mut _6: *mut u8;
scope 4 (inlined alloc::raw_vec::RawVec::<u8>::ptr) {
debug self => _2;
let mut _3: &alloc::raw_vec::RawVecInner;
scope 5 (inlined alloc::raw_vec::RawVecInner::ptr::<u8>) {
debug self => _3;
let mut _7: std::ptr::NonNull<u8>;
let mut _5: std::ptr::NonNull<u8>;
scope 6 (inlined alloc::raw_vec::RawVecInner::non_null::<u8>) {
debug self => _3;
let mut _4: std::ptr::NonNull<u8>;
let mut _2: std::ptr::NonNull<u8>;
scope 7 (inlined Unique::<u8>::cast::<u8>) {
debug ((self: Unique<u8>).0: std::ptr::NonNull<u8>) => _4;
debug ((self: Unique<u8>).1: std::marker::PhantomData<u8>) => const PhantomData::<u8>;
scope 8 (inlined NonNull::<u8>::cast::<u8>) {
debug self => _4;
let mut _5: *mut u8;
let mut _6: *const u8;
let mut _3: *mut u8;
let mut _4: *const u8;
scope 9 (inlined NonNull::<u8>::as_ptr) {
debug self => _4;
}
}
}
scope 10 (inlined Unique::<u8>::as_non_null_ptr) {
debug ((self: Unique<u8>).0: std::ptr::NonNull<u8>) => _7;
debug ((self: Unique<u8>).1: std::marker::PhantomData<u8>) => const PhantomData::<u8>;
}
}
scope 11 (inlined NonNull::<u8>::as_ptr) {
debug self => _7;
}
}
}
}
scope 12 (inlined std::slice::from_raw_parts::<'_, u8>) {
debug data => _9;
debug len => _10;
let _11: *const [u8];
debug data => _7;
debug len => _8;
let _9: *const [u8];
scope 13 (inlined core::ub_checks::check_language_ub) {
scope 14 (inlined core::ub_checks::check_language_ub::runtime) {
}
Expand All @@ -58,49 +46,42 @@ fn vec_deref_to_slice(_1: &Vec<u8>) -> &[u8] {
scope 16 (inlined align_of::<u8>) {
}
scope 17 (inlined slice_from_raw_parts::<u8>) {
debug data => _9;
debug len => _10;
debug data => _7;
debug len => _8;
scope 18 (inlined std::ptr::from_raw_parts::<[u8], u8>) {
debug data_pointer => _9;
debug metadata => _10;
debug data_pointer => _7;
}
}
}
}
}

bb0: {
StorageLive(_8);
StorageLive(_9);
StorageLive(_6);
StorageLive(_7);
StorageLive(_5);
StorageLive(_2);
_2 = &((*_1).0: alloc::raw_vec::RawVec<u8>);
_2 = copy (((((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner).0: std::ptr::Unique<u8>).0: std::ptr::NonNull<u8>);
StorageLive(_3);
_3 = &(((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner);
StorageLive(_7);
StorageLive(_4);
_4 = copy (((((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner).0: std::ptr::Unique<u8>).0: std::ptr::NonNull<u8>);
StorageLive(_5);
StorageLive(_6);
_5 = copy _4 as *mut u8 (Transmute);
_6 = copy _5 as *const u8 (PtrToPtr);
_7 = NonNull::<u8> { pointer: move _6 };
StorageDead(_6);
StorageDead(_5);
_3 = copy _2 as *mut u8 (Transmute);
_4 = copy _3 as *const u8 (PtrToPtr);
_5 = NonNull::<u8> { pointer: move _4 };
StorageDead(_4);
_8 = copy _7 as *mut u8 (Transmute);
StorageDead(_7);
StorageDead(_3);
_9 = copy _8 as *const u8 (PtrToPtr);
StorageDead(_2);
StorageLive(_10);
_10 = copy ((*_1).1: usize);
StorageLive(_11);
_11 = *const [u8] from (copy _9, copy _10);
_0 = &(*_11);
StorageDead(_11);
StorageDead(_10);
_6 = copy _5 as *mut u8 (Transmute);
StorageDead(_5);
_7 = copy _6 as *const u8 (PtrToPtr);
StorageLive(_8);
_8 = copy ((*_1).1: usize);
StorageLive(_9);
_9 = *const [u8] from (copy _7, copy _8);
_0 = &(*_9);
StorageDead(_9);
StorageDead(_8);
StorageDead(_7);
StorageDead(_6);
return;
}
}
Loading
Loading