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

Add nul-terminated filename for #[track_caller] #131828

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 4 additions & 1 deletion compiler/rustc_const_eval/src/util/caller_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ fn alloc_caller_location<'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 name = filename.as_str().to_string();
name.push('\0');

ecx.allocate_str(&name, 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.

Would be nice to mark the string as cstring in the compiled object files to allow the linker to deduplicate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that done by using a different MemoryKind?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we currently have a way to do this, but it would make sense to me to do it by adding a new MemoryKind and then special casing this in the codegen backends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to complicate this PR too much. That sounds like it could happen as a follow-up.

} else {
// FIXME: This creates a new allocation each time. It might be preferable to
// perform this allocation only once, and re-use the `MPlaceTy`.
Expand Down
66 changes: 53 additions & 13 deletions library/core/src/panic/location.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use crate::fmt;

#[cfg(not(bootstrap))]
use crate::ffi::CStr;

/// A struct containing information about the location of a panic.
///
/// This structure is created by [`PanicHookInfo::location()`] and [`PanicInfo::location()`].
Expand Down Expand Up @@ -32,7 +35,11 @@ use crate::fmt;
#[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[stable(feature = "panic_hooks", since = "1.10.0")]
pub struct Location<'a> {
file: &'a str,
// When not bootstrapping the compiler, it is an invariant that the last byte of this string
// slice is a nul-byte.
//
// When bootstrapping the compiler, this string may be missing the nul-terminator.
file_with_nul: &'a str,
line: u32,
col: u32,
}
Expand Down Expand Up @@ -127,7 +134,30 @@ impl<'a> Location<'a> {
#[rustc_const_stable(feature = "const_location_fields", since = "1.79.0")]
#[inline]
pub const fn file(&self) -> &str {
self.file
// String slicing in const is very hard, see:
// <https://users.rust-lang.org/t/slicing-strings-in-const/119836>

let s = self.file_with_nul;

#[cfg(bootstrap)]
if !matches!(s.as_bytes().last(), Some(0)) {
return s;
}

#[cfg(debug_assertions)]
if !matches!(s.as_bytes().last(), Some(0)) {
panic!("filename is not nul terminated");
}

// SAFETY: The string contains a nul-byte, so the length is at least one.
let len = unsafe { s.len().unchecked_sub(1) };

// SAFETY: `s.as_ptr()` is valid for `len+1` bytes, so it is valid for `len` bytes.
let file = unsafe { core::slice::from_raw_parts(s.as_ptr(), len) };

// SAFETY: This is valid utf-8 because the original string is valid utf-8 and the last
// character was a nul-byte, so removing it does not cut a codepoint in half.
unsafe { core::str::from_utf8_unchecked(file) }
}

/// Returns the line number from which the panic originated.
Expand Down Expand Up @@ -179,24 +209,34 @@ impl<'a> Location<'a> {
pub const fn column(&self) -> u32 {
self.col
}
}

#[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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this internal constructor does not seem to be used anymore.

Location { file, line, col }
/// Returns the name of the source file from which the panic originated as a nul-terminated
/// string.
///
/// This function is like [`Location::file`], except that it returns a nul-terminated string. It
/// is mainly useful for passing the filename into C or C++ code.
#[must_use]
#[inline]
#[unstable(feature = "panic_file_with_nul", issue = "none")]
#[cfg(not(bootstrap))]
pub fn file_with_nul(&self) -> &CStr {
let file_with_nul = self.file_with_nul.as_bytes();

#[cfg(debug_assertions)]
if !matches!(file_with_nul.last(), Some(0)) {
Darksonn marked this conversation as resolved.
Show resolved Hide resolved
panic!("filename is not nul terminated");
}

// SAFETY: This struct is only ever constructed by the compiler, which always inserts a
// nul-terminator in this string.
unsafe { CStr::from_bytes_with_nul_unchecked(file_with_nul) }
}
}

#[stable(feature = "panic_hook_display", since = "1.26.0")]
impl fmt::Display for Location<'_> {
#[inline]
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