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

[Darksonn]

Proposal

Problem statement

When using #[track_caller] in codebases that mix C and Rust, you often wish to pass the caller's filename to a C api. However, this usually requires a nul-terminated string.

Motivating examples or use cases

I 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.

Note that unlike with things like the file!() macro, it's impossible for us to do this ourselves statically. Copying the filename at runtime into another string to nul-terminate it is also not a great solution because we need to create the string even if the assertion doesn't fail, as the assertion is checked on the C side.

Solution sketch

Add a new function core::panic::Location::file_with_nul that returns a &CStr instead of a &str.

This has the implication that the compiler must now always store a nul-byte in the filename when generating the string constants.

Alternatives

It could make sense to return *const c_char instead of &CStr to avoid having to compute the length when all you need is a pointer you can pass into C code. This could be important as possible future work involves reducing the size of Location by removing the length. In this case, the existing core::panic::Location::file function would be updated to compute the length using the nul-terminator. Right now, the &CStr return value forces us to compute the length even when we don't need it.

Links and related work

An implementation can be found at rust-lang/rust#131828.

For more context, please see zulip and the Linux kernel mailing list. This is one of RfL's wanted features in core.

Adding a nul-terminator to the Location string has been tried before in rust-lang/rust#117431. However, back then, it was motivated by reducing the size of Location, and the previous PR did not actually expose the c string in the API.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

cc @ojeda @Noratrieb

[pitaj]

IIRC string constants are always null terminated in the binary anyways.

[Noratrieb]

I don't think they are

[programmerjake]

it's a constant with known length, I wouldn't consider "computing the length" to be a problem.
e.g.:

pub struct Location<'a> {
    file_with_nul: &'a [u8],
    line: u32,
    column: u32,
}

impl<'a> Location<'a> {
    pub fn file(&self) -> &'a str {
        unsafe { str::from_utf8_unchecked(self.file_with_nul.get_unchecked(..self.file_with_nul.len() - 1)) }
    }
    pub fn file_cstr(&self) -> &'a CStr {
        unsafe { CStr::from_bytes_with_nul_unchecked(self.file_with_nul) }
    }
}

[programmerjake]

one thing that changes though is that if we get an API to set the implicit #[track_caller] argument, that we'd have to pass a &'static [u8] in that has both the terminating nul and is utf-8, instead of the much more common &str

[Darksonn]

it's a constant with known length, I wouldn't consider "computing the length" to be a problem.

But it becomes a problem if we later go through with the size optimization from rust-lang/rust#117431. Then, the length is no longer known, so it really does have to be computed by calling strlen or similar.

[scottmcm]

It seems unfortunate if all the track_caller data in the binary needs to be bigger for everyone just because some people want to pass it to a random C API sometimes.

Could we have file_cstr!() instead, so only people dealing in C strings need to deal with it? Yes, that's not as nice as track_caller, but oh well?

[programmerjake]

if combined with the optimization in Location's size, it's probably smaller to use nul-terminated strings, since each string is only needed for a whole file and only needs one more byte whereas the size field is duplicated for every tracked location and is either 4 or 8 extra bytes in each one.

[Noratrieb]

Using null terminated strings may also unlock linker string merging size optimizations, which could further decrease binary size.
It seems unlikely to me that anyone cares about the tiny size increase - those who really really care about location size are gonna use something like -Zlocation-detail=none anyways, which deletes all this info.

[Darksonn]

Could we have file_cstr!() instead, so only people dealing in C strings need to deal with it? Yes, that's not as nice as track_caller, but oh well?

It would make a lot of things that could otherwise be function calls into macros. :(

[traviscross]

The libs-api team talked about this today on a short-staffed call. Those on the call had a question:

Looking at the motivating example, why not write a version of __might_sleep that takes a pointer and a length? What are the drawbacks to that?

As context, the feeling on the call was that this represents a tradeoff of whether to make the C codebase more Rust-like or Rust more C-like, and people weren't sure it was worth making Rust more C-like, and paying any costs here for all users, in this case.

It was noted on the call that this PR...

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

...had been closed as not being worth it. Though, reading the comments here now more closely, such as the one from @Noratrieb (who was the author of that PR) here, I gather that perhaps there is some interest in trying this again.

If there is a way to do this that does in fact result in a worthwhile improvement for all Rust users, then my own feeling is that probably would have affected the mood on the call about this proposal.

[workingjubilee]

I am not sure that there is in fact a "cost" here.

Every file path already has a de-facto terminator: it is suffixed by ".rs", and this causes it to be "prefix-free": https://en.wikipedia.org/wiki/Prefix_code

This is the same property possessed by NUL-terminated CStr. Each has "\0" at the end, which means no CStr can be a prefix of another CStr. Thus the argument about the cost seems wildly speculative, unless we wish to introduce a very curious new state of affairs, like not providing the ".rs" suffix!

Meanwhile, these file paths also could benefit from linker-driven deduplication (which revolves around the fact that CStrs can share a suffix).

[shepmaster]

Every file path already has a de-facto terminator: it is suffixed by ".rs",

./main
thread 'main' panicked at foo.js:2:5:
Where am I?
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

main.rs

#[path = "foo.js"]
mod bar;

fn main() {
    bar::bang();
}

foo.js

pub fn bang() {
    panic!("Where am I?");
}

---

_{Alas, I regret not picking the file extension .workingjubilee now...}

[workingjubilee]

@shepmaster this is true! there is actually afaik nothing preventing you from making all your files have the .shepmaster extension or anything else, aside from some wrangling. or potentially even eliminating extensions in such a way that removes the prefix-free property! it just so happens to be the case that this is not the case in current practical situations.

which I think would be the most interesting comparison point for this proposal: it would be a "hard left turn" in diagnostics for us to not report the exact file path, but we could compare this against something that does strip file extension suffixes and thus allows prefix-based coalescing of file paths into strings. this would work especially well with "Rust 2018" style import paths.

[saethlin]

I think if people are going to claim that this feature has a cost to all users of Rust, they should be able to demonstrate that cost measured by our benchmark suite. I have previously measured the overhead of the track_caller feature in rust-lang/rust#129704, so I've posted rust-lang/rust#132081 to collect some measurements of the overhead of adding this null terminator. I'd like to have that number just so that people know what it is.

There's another way out of this, depending on the vibes people are willing to tolerate. For interop with C generally, we could add a compiler flag that null-terminates track_caller strings. Users of the null-terminated API would have to build everything with the flag, and would just say location.file().as_ptr(), and the strlen for that pointer would be doing rust-lang/unsafe-code-guidelines#256.

---

Of course this can't happen on Linux, but has anyone thought about what happens when a user of this null-terminated API encounters an interior null byte?

---

strip file extension suffixes and thus allows prefix-based coalescing of file paths into strings

Well, considering these are always full paths, the only coalescing that I think is possible here is merging together a module root with its children, so these two files:

/home/ben/project/src/utils.rs
/home/ben/project/src/utils/inner.rs

I'd expect this to be rather low-yield, because it doesn't work at all in projects that use mod.rs, and only finds one merge per module.

[the8472]

Does the formatting machinery preassemble strings? I.e. format!("foo{}baz", "bar") gets turned into foobarbaz in the binary. Then &"bar" can be a substring of &"foobarbaz". So if file names get inlined into file strings then this substring-deduplication only works if there aren't stray zero terminators injected into "bar".

[saethlin]

The maximum binary size overhead measured in our benchmark suite is 0.04%, and most benchmarks reported the size change is below their significance threshold for binary size changes. rust-lang/rust#132081 (comment)

The average binary size increase is 0.01%.

[programmerjake]

did you also try removing the length field from Location at the same time? that may save a lot more space since you'll commonly have many Location structs per file but only one file name string

[saethlin]

There is already a perf run for that in the PR linked above, which I can link again here: rust-lang/rust#117431

[Darksonn]

Here's the thing. We don't truly need this for the kernel; it would be annoying to work around it, but we can change the C code and have it accept a length instead of a nul-terminated string.

However, I think it's an unfortunate direction because adding Rust to existing C/C++ codebases is the future, and if we want Rust to work well in such codebases, we have to admit that they are not just going to give up their nul-terminated strings. I'm certain that this feature (or something equivalent) is not limited to only being useful in the kernel, and that any other codebase that does C/C++ interop will require this feature in order to have good error messages when something goes wrong when Rust calls into C/C++.

After all, when Rust is in the minority in a codebase, we want the experience of using Rust there to be good. Having the Rust side give worse error messages than you get in C/C++ is not a good look. But that is the current state in the kernel right now, and fixing it would be a lot easier with this feature.

[RalfJung]

Meanwhile, these file paths also could benefit from linker-driven deduplication (which revolves around the fact that CStrs can share a suffix).

This kind of deduplication should work even without a trailing nul, right? Suffixes can be shared either way.

[tgross35]

Meanwhile, these file paths also could benefit from linker-driven deduplication (which revolves around the fact that CStrs can share a suffix).

This kind of deduplication should work even without a trailing nul, right? Suffixes can be shared either way.

I wondered about that before too: apparently this is a (difficult to surmount) linker limitation, not something intrinsic https://rust-lang.zulipchat.com/#narrow/channel/405744-wg-binary-size/topic/.E2.9C.94.20Surprisingly.20big.20win.20from.20nul-terminating.20string.20constants

[RalfJung]

Wow that's extraordinarily silly. I don't think we should engineer anything long-term around such an accident of history, and instead work with linkers to improve their deduplication to not be so C-centric.

[BurntSushi]

For me personally, at the API level, I'm totally fine with adding a file_with_nul method that returns a &CStr to Location. From an API perspective, it seems like a small thing. And it improves interoperability. The "always compute length" seems like a bummer, but does this get fixed if and when CStr moves its length calculation from construction to consumption? (I'm unsure of the status of that idea, but I think it's been "the plan" for a long time.)

In terms of whether this is worth the extra overhead, I'm less sure. Based on the reported overhead figures above, I'm inclined to say yes. But I would want to ensure that the compiler team is on the FCP of this API if this ACP were to move forward to that point.

[cramertj]

I independently opened rust-lang/rust#135054 implementing this feature (initially as Location::file_cstr) without realizing that there was an existing proposal here. I've since renamed it to file_with_nul to match this proposal.

It looks like the current process first requires this issue to be accepted. Are there remaining blockers, or can this move forward? The perf and binary size impact don't seem significant.

Could we have file_cstr!() instead, so only people dealing in C strings need to deal with it? Yes, that's not as nice as track_caller, but oh well?

For my use-cases at least, a file_cstr! macro without #[track_caller] support wouldn't solve the problem, as it would require any C++ interop APIs which collect a source location to be implemented as macros and would prevent APIs like absl::{Status, StatusOr}'s usage with ?.

[BurntSushi]

I'd be inclined to accept this ACP, but it seems like when libs-api last discussed it, reception was lukewarm (but doesn't read as definitive to me).

If someone else on libs-api wants to second this, then I think that would be good enough to move forward.

Caveats:

• Accepting this ACP doesn't mean the discussion about the relative merits of the increased binary size has to stop. Although, I do wonder if this cost has to be paid even while the API is unstable.
• I think the compiler team should have some input here. Not necessarily now, but before (or as part of) stabilization.

[Noratrieb]

The cost has to be paid even while it's unstable, but we can always remove it if the cost is seen as a problem. Personally, I'm really not worried about the cost at all.

[the8472]

I'm more worried that perf/footprint looks ok today because linkers don't deduplicate hard enough and that adding this would block improvements. Though I don't know what linkers do and don't support.
See my previous comment: #466 (comment)

[programmerjake]

i don't expect linker deduplication to be able to do much (other than storing each file name once, but I assume rustc can do that itself) since file paths are generally not substrings of other file paths -- they can share prefixes or share suffixes but generally not both

[the8472]

I'd expect file paths (without nul terminator) to be substrings of formatted strings such as panic messages. So paths with nul terminator can't be substring-deduped with those.

[saethlin]

I'd expect file paths (without nul terminator) to be substrings of formatted strings such as panic messages.

Really? Are people using file!() in panic messages or something like that? The whole system that we're discussing here exists to put paths in panic messages so I would find it quite strange if people are also doing that manually.

[cramertj]

i don't expect linker deduplication to be able to do much (other than storing each file name once, but I assume rustc can do that itself) since file paths are generally not substrings of other file paths -- they can share prefixes or share suffixes but generally not both

Note that my PR already implements this (only stores the file path once).

[the8472]

I'd expect file paths (without nul terminator) to be substrings of formatted strings such as panic messages.

Really? Are people using file!() in panic messages or something like that? The whole system that we're discussing here exists to put paths in panic messages so I would find it quite strange if people are also doing that manually.

The log crate tracks Location for each log call. I don't know if they optimize down to a preformatted string, but at least it's a conceivable optimization.

[saethlin]

So is your concern that if a string from Location::file gets optimized into another string, that it then can't be deduplicated with a Location::file string that isn't optimized into another string?

[Darksonn]

I think such optimizations are exceedingly unlikely in practice.

[the8472]

Wouldn't it just be const-folding plus whatever optimizations the formatting machinery is already applying to build those compile-time-concatented strings?

[tgross35]

It combines formatting arguments at an AST level, I don't think there is any const prop so Location's format string is unlikely to be inlined https://rust.godbolt.org/z/aW4zKTh4e. That could be interesting, the current argument flattening came with a runtime and binary size win rust-lang/rust#109999.

In any case, this API should probably be looked at only from an interop standpoint, not for any possible size wins at link time. Just using trim-paths / remap-path-scope probably has bigger wins for the majority of users since it can eliminate the common prefix.

The hash in std's traces could also likely be shortened, it's 50% of the entire path...

stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/917bfa78478cbcc77406e5ea37b24c3eedefacf4/library/std/src/panicking.rs:748:5

[tmandry]

I'm sympathetic to the point that there might be a future cost to this that we don't realize. But after thinking about it more, I'm not worried: I think the worst case scenario is that we come up with some very compelling way to do string optimizations in the future that are incompatible with this API, and we decide (because this API exists) that they should require an opt-in via a compiler flag which causes this API to always return an empty string. That would not be so bad; it's in line with the existing flags we provide to remove location info from binaries.

Even that seems unlikely to me though, given the points raised by @workingjubilee and the difficulty in coming up with a plausible optimization that would be defeated in this thread.

It does seem like we might want to cut down on the size of these strings in the future. Maybe there's a way we can optimize location info with some tree structure baked into the binary (or off-the-shelf compression) and reifying a string at runtime, but I don't believe the addition of this API precludes that. Location::caller() returning a &'static Location creates a problem here, since we have no way to reclaim the memory of the reified string once Location::file() is called and would have to leak it, but that's preexisting.

[Darksonn]

I added a PR with the alternative option of adding an unstable flag: rust-lang/rust#135240.

[Noratrieb]

I did a rustc-perf run on the relevant PR to measure the concrete binary size impact: rust-lang/rust#135054 (comment)

It confirms what I suspected. While the binaries do get bigger of course, rustc-perfs rounding to one decimal place rounds it to 0.0%, making it very clear that it's completely negligible.

Now there are people who really really care about binary size. But this one byte will definitely not push then over the edge. No, they won't want any of this location information, they will use the unstable (or potentially stable in the future) -Zlocation-detail=none flag to remove all the information, which is what will actually satisfy their needs.
We do not need to care about them here.

Therefore, I really recommend to accept this ACP, at least if the upsides are considered useful (which I think they are), as the downsides of binary size can be ignored.

bonus: with a small sacrifice of speed on accesses, we could even improve binary size with this with the scheme described in rust-lang/rust#117431. we don't have to of course, and I don't think that should be part of the discussion