Skip to content
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

Null terminate core::panic::Location file strings #117431

Closed
wants to merge 1 commit into from
Closed
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
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 {
Copy link
Member

Choose a reason for hiding this comment

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

Uh, this is bad. That code is already terrible and my attempts at cleaning it up in #116745 are kind of stuck. I really don't like adding more complications here.

oli-obk marked this conversation as resolved.
Show resolved Hide resolved
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()
};
Copy link
Member

Choose a reason for hiding this comment

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

I think you can avoid the interning change by instead manually interning the inner allocation with the string here.

Copy link
Member Author

Choose a reason for hiding this comment

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

that sounds good. how exactly do i do the interning? is it the

            let alloc = tcx.mk_const_alloc(alloc);
            tcx.set_alloc_id_memory(alloc_id, alloc);

part?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's the core thing. We have some helpers in interpret/intern.rs. Possible one of the existing one needs extension, currently there's basically 2 helpers for 2 users but it'd be good to start establishing some patterns. (That can also happen later though.)

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 (),
Copy link
Member

Choose a reason for hiding this comment

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

Needs an explicit Send/Sync impl now to keep the same API.

_file_actual: PhantomData<&'a CStr>,
Copy link
Member

Choose a reason for hiding this comment

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

Why does Location have a lifetime? Isn't it always 'static?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know either - but it's stable :(

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))
Copy link
Member

Choose a reason for hiding this comment

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

This makes calls here and to related (Hash, Debug, PartialEq, etc.) functions much more expensive. I guess we may not be able to do much better... but I think the binary size vs. possibly slower binaries that are keeping these locations around (e.g., HashMap) isn't as obvious.

It's possible we could make this near equivalent by avoiding the upfront 0-byte search internally.

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be doing a varint length prefix instead of a nul terminator. For short paths it would only be 1 byte so the same size as a nul terminator.

Copy link
Member

Choose a reason for hiding this comment

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

Are these ever on the hot path?

Copy link
Member

Choose a reason for hiding this comment

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

Logging, maybe?

}
}

/// 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)
}
}
Loading