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

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented Oct 17, 2024

ACP: Add nul-terminated version of core::panic::Location::file

When using #[track_caller] you can get the filename of the caller using Location::caller().file(). We would like to utilize this in the Linux kernel to implement a Rust equivalent of the following utility:

/**
 * might_sleep - annotation for functions that can sleep
 *
 * this macro will print a stack trace if it is executed in an atomic
 * context (spinlock, irq-handler, ...). Additional sections where blocking is
 * not allowed can be annotated with non_block_start() and non_block_end()
 * pairs.
 *
 * This is a useful debugging help to be able to catch problems early and not
 * be bitten later when the calling function happens to sleep when it is not
 * supposed to.
 */
#define might_sleep() do { __might_sleep(__FILE__, __LINE__); might_resched(); } while (0)

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, extend Location 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

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 17, 2024
)]
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.

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.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

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)

@Darksonn
Copy link
Contributor Author

Darksonn commented Oct 17, 2024

Yeah, getting rid of the length could reduce the size of Location. Not sure how to do that properly considering the bootstrapping complications, though.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

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.

@Darksonn
Copy link
Contributor Author

Based on the above failure, it seems like my allocation stuff is also wrong? Does it get allocated in multiple places?

@Noratrieb
Copy link
Member

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

@Darksonn
Copy link
Contributor Author

@Noratrieb Nevermind, looks like I was just missing a nul-terminator in the <redacted> string. Seems like the code works as-is. Is there any issue with the current bootstrapping approach?

@Noratrieb
Copy link
Member

it seems correct to me. i guess it makes more sense to do the size reduction separately in the future.
r? Noratrieb
could you open an ACP for the new method with the implication of now always storing the extra nul? I think that would make sense here.

@rustbot rustbot assigned Noratrieb and unassigned jhpratt Oct 17, 2024
@nikomatsakis
Copy link
Contributor

This seems like a good idea!

Copy link
Member

@jhpratt jhpratt left a 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.

@Noratrieb Noratrieb added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2024
@bors
Copy link
Contributor

bors commented Dec 10, 2024

☔ The latest upstream changes (presumably #134096) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants