-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Alice Ryhl <[email protected]>
)] | ||
impl<'a> Location<'a> { | ||
#[doc(hidden)] | ||
pub const fn internal_constructor(file: &'a str, line: u32, col: u32) -> Self { |
There was a problem hiding this comment.
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.
let mut name = filename.as_str().to_string(); | ||
name.push('\0'); | ||
|
||
ecx.allocate_str(&name, MemoryKind::CallerLocation, Mutability::Not).unwrap() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
see #117431, this is a small binary size improvement if we then make use of the nul terminator (probably only worth it after the bootstrap bump) |
Yeah, getting rid of the length could reduce the size of |
This comment has been minimized.
This comment has been minimized.
My PR implements the bootstrapping part correctly (including the place where the Location constant is created), but the whole byte allocation stuff is wrong. I recommend you copy the library part and Location creation from my PR and keep the string allocator from yours. |
Based on the above failure, it seems like my allocation stuff is also wrong? Does it get allocated in multiple places? |
hm, not sure. not understanding the problems with the allocation stuff is why I quit my PR, so I can't help you there sadly :3 |
@Noratrieb Nevermind, looks like I was just missing a nul-terminator in the |
it seems correct to me. i guess it makes more sense to do the size reduction separately in the future. |
This seems like a good idea! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me if/when the ACP gets accepted.
☔ The latest upstream changes (presumably #134096) made this pull request unmergeable. Please resolve the merge conflicts. |
ACP: Add nul-terminated version of core::panic::Location::file
When using
#[track_caller]
you can get the filename of the caller usingLocation::caller().file()
. We would like to utilize this in the Linux kernel to implement a Rust equivalent of the following utility:It's essentially an assertion that crashes the kernel if a function is used in the wrong context. The filename and line number is used in the error message when it fails. Unfortunately, the
__might_sleep
function requires the filename to be a nul-terminated string. Thus, extendLocation
with a function that returns the filename as a&CStr
.Note that unlike with things like the
file!()
macro, it's impossible for us to do this ourselves statically. Copying the filename into another string to nul-terminate it is not a great solution because we need to create the string even if the assertion doesn't fail, as the assertion happens on the C side.For more context, please see zulip and the Linux kernel mailing list. This is one of RfL's wanted features in core.
cc @ojeda @Urgau