Skip to content

Commit

Permalink
Auto merge of #117431 - Nilstrieb:nul-location, r=<try>
Browse files Browse the repository at this point in the history
Null terminate `core::panic::Location` file strings

This shrinks `Location` by a usize, making it 24->16 bytes on x86-64. This reduces binary size. I quickly measure `cargo install ripgrep`, which was reduced by 0.3%. That's not a lot, but it's something and the code change is quite simple. From some quick measurements, I expect librustc_driver to shrink by more, but rustc-perf should give us more numbers.

The change to const interning is necessary, see https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/unsupported.20untyped.20pointer.20in.20constant.20error/near/399446285
  • Loading branch information
bors committed Oct 31, 2023
2 parents 22b2712 + fc2f71f commit c5474df
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 20 deletions.
11 changes: 7 additions & 4 deletions compiler/rustc_const_eval/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ pub fn intern_const_alloc_recursive<
debug!(?todo);
debug!("dead_alloc_map: {:#?}", ecx.memory.dead_alloc_map);
while let Some(alloc_id) = todo.pop() {
if let Some((_, mut alloc)) = ecx.memory.alloc_map.remove(&alloc_id) {
if let Some((alloc_kind, mut alloc)) = ecx.memory.alloc_map.remove(&alloc_id) {
// We can't call the `intern_shallow` method here, as its logic is tailored to safe
// references and a `leftover_allocations` set (where we only have a todo-list here).
// So we hand-roll the interning logic here again.
Expand Down Expand Up @@ -424,9 +424,12 @@ pub fn intern_const_alloc_recursive<
// something that cannot be promoted, which in constants means values that have
// drop glue, such as the example above.
InternKind::Constant => {
ecx.tcx.sess.emit_err(UnsupportedUntypedPointer { span: ecx.tcx.span });
// For better errors later, mark the allocation as immutable.
alloc.mutability = Mutability::Not;
// The Location does have leftover allocations, but that's fine, as we control it.
if alloc_kind != MemoryKind::CallerLocation {
ecx.tcx.sess.emit_err(UnsupportedUntypedPointer { span: ecx.tcx.span });
// For better errors later, mark the allocation as immutable.
alloc.mutability = Mutability::Not;
}
}
}
let alloc = tcx.mk_const_alloc(alloc);
Expand Down
21 changes: 16 additions & 5 deletions compiler/rustc_const_eval/src/util/caller_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use rustc_middle::mir;
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::{source_map::DUMMY_SP, symbol::Symbol};
use rustc_target::abi::Align;
use rustc_type_ir::Mutability;

use crate::const_eval::{mk_eval_cx, CanAccessStatics, CompileTimeEvalContext};
Expand All @@ -19,13 +20,23 @@ fn alloc_caller_location<'mir, 'tcx>(
// This can fail if rustc runs out of memory right here. Trying to emit an error would be
// pointless, since that would require allocating more memory than these short strings.
let file = if loc_details.file {
ecx.allocate_str(filename.as_str(), MemoryKind::CallerLocation, Mutability::Not).unwrap()
let mut bytes = filename.as_str().to_owned().into_bytes();
bytes.push(0);
ecx.allocate_bytes_ptr(&bytes, Align::ONE, MemoryKind::CallerLocation, Mutability::Not)
.unwrap()
} else {
// FIXME: This creates a new allocation each time. It might be preferable to
// perform this allocation only once, and re-use the `MPlaceTy`.
// See https://github.com/rust-lang/rust/pull/89920#discussion_r730012398
ecx.allocate_str("<redacted>", MemoryKind::CallerLocation, Mutability::Not).unwrap()
ecx.allocate_bytes_ptr(
b"<redacted>\0",
Align::ONE,
MemoryKind::CallerLocation,
Mutability::Not,
)
.unwrap()
};
let file = Scalar::from_pointer(file, ecx);
let line = if loc_details.line { Scalar::from_u32(line) } else { Scalar::from_u32(0) };
let col = if loc_details.column { Scalar::from_u32(col) } else { Scalar::from_u32(0) };

Expand All @@ -38,11 +49,11 @@ fn alloc_caller_location<'mir, 'tcx>(
let location = ecx.allocate(loc_layout, MemoryKind::CallerLocation).unwrap();

// Initialize fields.
ecx.write_immediate(file.to_ref(ecx), &ecx.project_field(&location, 0).unwrap())
ecx.write_scalar(file, &ecx.project_field(&location, 0).unwrap())
.expect("writing to memory we just allocated cannot fail");
ecx.write_scalar(line, &ecx.project_field(&location, 1).unwrap())
ecx.write_scalar(line, &ecx.project_field(&location, 2).unwrap())
.expect("writing to memory we just allocated cannot fail");
ecx.write_scalar(col, &ecx.project_field(&location, 2).unwrap())
ecx.write_scalar(col, &ecx.project_field(&location, 3).unwrap())
.expect("writing to memory we just allocated cannot fail");

location
Expand Down
1 change: 1 addition & 0 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
#![feature(const_caller_location)]
#![feature(const_cell_into_inner)]
#![feature(const_char_from_u32_unchecked)]
#![feature(const_cstr_from_ptr)]
#![feature(const_eval_select)]
#![feature(const_exact_div)]
#![feature(const_float_bits_conv)]
Expand Down
89 changes: 78 additions & 11 deletions library/core/src/panic/location.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::fmt;
#[cfg(not(bootstrap))]
use crate::{ffi::CStr, marker::PhantomData};

/// A struct containing information about the location of a panic.
///
Expand Down Expand Up @@ -28,8 +30,21 @@ use crate::fmt;
/// Files are compared as strings, not `Path`, which could be unexpected.
/// See [`Location::file`]'s documentation for more discussion.
#[lang = "panic_location"]
#[derive(Copy, Clone)]
#[stable(feature = "panic_hooks", since = "1.10.0")]
#[cfg(not(bootstrap))]
pub struct Location<'a> {
file: *const (),
_file_actual: PhantomData<&'a CStr>,
line: u32,
col: u32,
}

/// Location
#[lang = "panic_location"]
#[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[stable(feature = "panic_hooks", since = "1.10.0")]
#[cfg(bootstrap)]
pub struct Location<'a> {
file: &'a str,
line: u32,
Expand Down Expand Up @@ -126,7 +141,17 @@ impl<'a> Location<'a> {
#[rustc_const_unstable(feature = "const_location_fields", issue = "102911")]
#[inline]
pub const fn file(&self) -> &str {
self.file
#[cfg(bootstrap)]
{
self.file
}
#[cfg(not(bootstrap))]
// SAFETY: Codegen ensures that there is a null byte.
unsafe {
let cstr = CStr::from_ptr(self.file as _);
let len = cstr.count_bytes();
crate::str::from_utf8_unchecked(crate::slice::from_raw_parts(self.file as _, len))
}
}

/// Returns the line number from which the panic originated.
Expand Down Expand Up @@ -180,21 +205,63 @@ impl<'a> Location<'a> {
}
}

#[unstable(
feature = "panic_internals",
reason = "internal details of the implementation of the `panic!` and related macros",
issue = "none"
)]
impl<'a> Location<'a> {
#[doc(hidden)]
pub const fn internal_constructor(file: &'a str, line: u32, col: u32) -> Self {
Location { file, line, col }
// cfg(not(bootstrap)) NOTE: inline this module when bumping, it's only here to group the cfg
#[cfg(not(bootstrap))]
mod location_impls {
use super::Location;

impl Location<'_> {
fn to_parts(&self) -> (&str, u32, u32) {
(self.file(), self.line, self.col)
}
}

#[stable(feature = "panic_hooks", since = "1.10.0")]
impl crate::fmt::Debug for Location<'_> {
fn fmt(&self, f: &mut crate::fmt::Formatter<'_>) -> crate::fmt::Result {
f.debug_struct("Location")
.field("file", &self.file())
.field("line", &self.line)
.field("col", &self.col)
.finish()
}
}

#[stable(feature = "panic_hooks", since = "1.10.0")]
impl crate::cmp::PartialEq for Location<'_> {
fn eq(&self, other: &Self) -> bool {
self.to_parts() == other.to_parts()
}
}

#[stable(feature = "panic_hooks", since = "1.10.0")]
impl crate::cmp::Eq for Location<'_> {}

#[stable(feature = "panic_hooks", since = "1.10.0")]
impl crate::cmp::PartialOrd for Location<'_> {
fn partial_cmp(&self, other: &Self) -> Option<crate::cmp::Ordering> {
self.to_parts().partial_cmp(&other.to_parts())
}
}

#[stable(feature = "panic_hooks", since = "1.10.0")]
impl crate::cmp::Ord for Location<'_> {
fn cmp(&self, other: &Self) -> crate::cmp::Ordering {
self.to_parts().cmp(&other.to_parts())
}
}

#[stable(feature = "panic_hooks", since = "1.10.0")]
impl crate::hash::Hash for Location<'_> {
fn hash<H: crate::hash::Hasher>(&self, state: &mut H) {
self.to_parts().hash(state)
}
}
}

#[stable(feature = "panic_hook_display", since = "1.26.0")]
impl fmt::Display for Location<'_> {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(formatter, "{}:{}:{}", self.file, self.line, self.col)
write!(formatter, "{}:{}:{}", self.file(), self.line, self.col)
}
}

0 comments on commit c5474df

Please sign in to comment.