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 Location::file_with_nul #135054

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
21 changes: 13 additions & 8 deletions compiler/rustc_const_eval/src/util/caller_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,20 @@ fn alloc_caller_location<'tcx>(
line: u32,
col: u32,
) -> MPlaceTy<'tcx> {
// Ensure that the filename itself does not contain nul bytes.
// This isn't possible via POSIX or Windows, but we should ensure no one
// ever does such a thing.
assert!(!filename.as_str().as_bytes().contains(&0));

let loc_details = ecx.tcx.sess.opts.unstable_opts.location_detail;
// 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_dedup(filename.as_str()).unwrap()
} else {
ecx.allocate_str_dedup("<redacted>").unwrap()
let file_wide_ptr = {
let filename = if loc_details.file { filename.as_str() } else { "<redacted>" };
let filename_with_nul = filename.to_owned() + "\0";
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
// 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_ptr = ecx.allocate_bytes_dedup(filename_with_nul.as_bytes()).unwrap();
Immediate::new_slice(file_ptr.into(), filename_with_nul.len().try_into().unwrap(), ecx)
};
cramertj marked this conversation as resolved.
Show resolved Hide resolved
let file = file.map_provenance(CtfeProvenance::as_immutable);
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 @@ -37,7 +42,7 @@ fn alloc_caller_location<'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_immediate(file_wide_ptr, &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())
.expect("writing to memory we just allocated cannot fail");
Expand Down
53 changes: 38 additions & 15 deletions library/core/src/panic/location.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#[cfg(not(bootstrap))]
use crate::ffi::CStr;
use crate::fmt;

/// A struct containing information about the location of a panic.
Expand Down Expand Up @@ -32,7 +34,16 @@ use crate::fmt;
#[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[stable(feature = "panic_hooks", since = "1.10.0")]
pub struct Location<'a> {
#[cfg(bootstrap)]
file: &'a str,

// Note: this filename will have exactly one nul byte at its end, but otherwise
// it must never contain interior nul bytes. This is relied on for the conversion
// to `CStr` below.
//
// The prefix of the string without the trailing nul byte will be a regular UTF8 `str`.
#[cfg(not(bootstrap))]
file_bytes_with_nul: &'a [u8],
line: u32,
col: u32,
}
Expand Down Expand Up @@ -125,9 +136,33 @@ impl<'a> Location<'a> {
#[must_use]
#[stable(feature = "panic_hooks", since = "1.10.0")]
#[rustc_const_stable(feature = "const_location_fields", since = "1.79.0")]
#[inline]
pub const fn file(&self) -> &str {
self.file
#[cfg(bootstrap)]
{
self.file
}

#[cfg(not(bootstrap))]
{
let str_len = self.file_bytes_with_nul.len() - 1;
// SAFETY: `file_bytes_with_nul` without the trailing nul byte is guaranteed to be
// valid UTF8.
unsafe { crate::str::from_raw_parts(self.file_bytes_with_nul.as_ptr(), str_len) }
Comment on lines +147 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

str is allowed to have null bytes within. So you could keep the type at &'a str and use &self.file[..self.file.len() -1] here without unsafe code

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 could! I think that just moves the unsafety into a call to from_bytes_with_nul_unchecked for the CStr, though, so I think it's roughly the same either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

That unsafety exists either way unless you just directly encode it as CStr, tho we're still holding out for that becoming a thin pointer, at which point it would be expensive to get the str out of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's simpler to just store a raw pointer and length without the nul-terminator included. That way, Location hold the integer we want most of the time without triggering provenance concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

That way, Location hold the integer we want most of the time without triggering provenance concerns.

What integer do you mean?

I wasn't aware of any provenance concerns, was that discussed in #131828?

Copy link
Member

Choose a reason for hiding this comment

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

The provenance discussion is in a hidden subthread above: #135054 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that storing a raw pointer / length instead of a slice would let us store the length without the nul-terminator included, which makes more sense to me.

}
}

/// Returns the name of the source file as a nul-terminated `CStr`.
///
/// This is useful for interop with APIs that expect C/C++ `__FILE__` or
/// `std::source_location::file_name`, both of which return a nul-terminated `const char*`.
#[cfg(not(bootstrap))]
#[must_use]
#[unstable(feature = "file_with_nul", issue = "none")]
#[inline]
pub const fn file_with_nul(&self) -> &CStr {
// SAFETY: `file_bytes_with_nul` is guaranteed to have a trailing nul byte and no
// interior nul bytes.
unsafe { CStr::from_bytes_with_nul_unchecked(self.file_bytes_with_nul) }
}

/// Returns the line number from which the panic originated.
Expand Down Expand Up @@ -181,22 +216,10 @@ 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 }
}
}

#[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)
}
}
25 changes: 25 additions & 0 deletions tests/ui/rfcs/rfc-2091-track-caller/file-is-nul-terminated.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//@ run-pass
#![feature(file_with_nul)]

#[track_caller]
const fn assert_file_has_trailing_zero() {
let caller = core::panic::Location::caller();
let file_str = caller.file();
let file_with_nul = caller.file_with_nul();
if file_str.len() != file_with_nul.count_bytes() {
panic!("mismatched lengths");
}
let trailing_byte: core::ffi::c_char = unsafe {
*file_with_nul.as_ptr().offset(file_with_nul.count_bytes() as _)
};
if trailing_byte != 0 {
panic!("trailing byte was nonzero")
}
}

#[allow(dead_code)]
const _: () = assert_file_has_trailing_zero();

fn main() {
assert_file_has_trailing_zero();
}
Loading