library: Move CStr to libcore, and CString to liballoc

[petrochenkov]

Closes #46736

Interesting points:

• Stability:
  • To make CStr(ing) from libcore/liballoc unusable without enabling features I had to make these structures unstable, and reexport them from libstd using stable type aliases instead of pub use reexports. (Because stability of use items is not checked.)
• Relying on target ABI in libcore is ok:
  • library: Move CStr to libcore, and CString to liballoc #94079 (comment)
• trait CStrExt (UPDATE: used only in cfg(bootstrap) mode, otherwise lang items are used instead)
  • library: Move CStr to libcore, and CString to liballoc #94079 (comment)
• strlen
  • library: Move CStr to libcore, and CString to liballoc #94079 (comment)

Otherwise it's just a code move + some minor hackery usual for liballoc in cfg(test) mode.

[rust-highfive]

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

[joshtriplett]

This is great! I'd love to see this move, to make it easier to interface with C FFI in contexts that don't necessarily want std.

Platform strlen implementations typically have extensive optimizations, and I don't think we'd want to miss out on those. It might make sense to ensure we have a weak strlen in compiler-builtins or similar. (cc @Amanieu)

[petrochenkov]

Relying on target ABI in libcore is ok

A lot of attention in #46736 is put on whether depending on c_char in libcore is an acceptable thing or not.
I don't think the self-imposed restriction of not doing that is reasonable.

Obviously, libcore cannot depend on calling OS services, be it syscalls or functions or system libraries.
But why cannot it rely on the target's ABI (which includes sizes/alignments/signedness of basic C types)? It doesn't introduce any new dependencies to libcore.

Even without libcore the compiler cares about the target's ABI and keeps the relevant data in target specifications (inside rustc and LLVM).
It must know these things to generate layouts for repr(C) data structures, and calling conventions for generating extern "C" functions.
rustc's target specs, for example, have fields like c_int_width and c_enum_min_bits, even if we don't count the LLVM part.
So libcores build for different targets are already different, because the compiler uses that ABI knowledge when generating code.
If the compiler can do it, then why a higher layer like libcore cannot?

Recently core contains more of target-dependent stuff than back in 2017 when #46736 was created, because it's simply too useful to break that restriction sometimes.
A bunch of code in core::arch depends on the target's CPU architecture, and the va_list implementation in core::ffi requires relying on even more ABI details.

I personally don't see issues with putting that cfg_if that defines c_char into libcore, but perhaps people will feel better about this if the char signedness is moved to the compiler's target specs (compiler\rustc_target\src\spec\*.rs)?
Then the c_char definition will look roughly like

#[cfg(target_c_char_signed)]
type c_char = i8;
#[cfg(not(target_c_char_signed))]
type c_char = u8;

[Amanieu]

Platform strlen implementations typically have extensive optimizations, and I don't think we'd want to miss out on those. It might make sense to ensure we have a weak strlen in compiler-builtins or similar. (cc @Amanieu)

I am generally in favor of depending on more libc symbols in core (strlen, memchr, bzero, etc) while providing fallbacks in compiler-builtins for platforms that don't have that symbol.

[petrochenkov]

trait CStrExt

Two inherent methods on CStr (into_c_string and to_string_lossy) have signatures including types unavailable in libcore (CString, Cow and Box).

To keep backward compatibility I had to add an unstable trait CStrExt with these two stable methods, and add it to prelude.
That trait is only implemented for CStr.

strlen

CStr(ing) needs to calculate lengths of zero-terminated byte arrays.
While this functionality is trivial, its implementation can also be optimized by using CPU architecture specific techniques.
Such optimized implementations usually live in platform's libc, this is where the current libstd takes them from.

libcore doesn't have access to platform's libc, so it has to either

• Use the naive trivial implementation of strlen, or
• Provide the optimized version of strlen, similarly to how it does with memchr, or
• Use some scheme with weak symbols as suggested above, with weak naive implementation being provided by libcore by default, but then overridable by any other linked libraries, including platform's libc.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api team meeting. Many people are enthusiastic to see this work happen.

Regarding relying on target ABI, there were zero objections.

Regarding strlen, there was a consensus in favor of the general approach @Amanieu suggested, of going ahead and using platform C library functions in core as long as they're functions that we can fall back to compiler-builtins for, so that we can use the optimized implementations from the C library if available.

Regarding CStr functions that return types only available in alloc or std: we had some discussion about whether we should use an extension trait for this, or use a lang item or some similar mechanism to allow extending CStr from alloc and std. Our primary concern there was that an extension trait may technically be a very slightly breaking change, if someone had a different trait with the same method names; extension trait vs inherent method has different precedence. Using lang items to extend the set of inherent methods from alloc wouldn't have that issue.

Ultimately, though, we decided that we'd rather not add lang items for this, and we'd rather use an extension trait as long as that won't break anything in practice. So, summary: let's do a crater run to confirm that the extension trait doesn't break anything, and then go with the extension trait.

[petrochenkov]

we had some discussion about whether we should use an extension trait for this, or use a lang item or some similar mechanism to allow extending CStr from alloc and `std

When implementing this I recalled that we had such extension traits before, but I couldn't find any of them in libcore now.
Apparently all of them were converted to lang items in 2018 (the main PR was #49896), so the existing policy seems to be preferring lang items for this.

This CStr(ing) move would require a single new lang item in liballoc.

[joshtriplett]

@petrochenkov A lang item seems fine as well. And (EDIT: corrected) we don't need a crater run in that case.

[bstrie]

Note that #94503 has just moved c_char (among others) to core::ffi.

[joshtriplett]

We discussed this again in today's @rust-lang/libs meeting. We're fine with using a lang item in this case, since that's what we already do in other cases.

In general, we'd like to steer away in the future from having any inherent methods on types in core that return types from alloc or std (or inherent methods on types in alloc that return types from std, if any). Such methods make the binding between those two types more special, and make it a little harder to write drop-in replacements for types like CString/String/etc. So, for instance, in this case we should ideally steer people towards into() rather than into_c_string(), so that people can just impl From<CStr> for MyCStringType.

But none of that is a blocker for this move; this method can go ahead and use a lang item as other methods do, and we'll have the more general discussion in the future.

[petrochenkov]

@bors r=joshtriplett

[bors]

📌 Commit 6eaec56 has been approved by joshtriplett

[bors]

⌛ Testing commit 6eaec56 with merge f50f07b174bc2022c40eefc1b9b5f6ade8b4505a...

[petrochenkov]

@bors r=joshtriplett

[bors]

📌 Commit f62c84e has been approved by joshtriplett

[bors]

⌛ Testing commit f62c84e with merge 1e6fe58...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 1e6fe58 to master...

[rust-timer]

Finished benchmarking commit (1e6fe58): comparison url.

Summary:

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	3	0	2	1
mean2	0.8%	0.9%	N/A	-0.6%	0.8%
max	0.8%	1.2%	N/A	-0.7%	0.8%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[petrochenkov]

@lygstate
This PR implements the strategy from #94079 (comment).
If libc is linked then strlen is used from there (this is typically a fast platform-provided implementation), otherwise it's used from compiler-builtins (this is a naive implementation).

If you have issues with this setup, could you open a new issue and describe your case?

[pnkfelix]

Visiting for weekly performance triage

• The report above claims this caused a 0.8% regression on primary benchmark unicode-normalization-0.1.19 (check full).
• I was not able to reproduce the regression locally; all my runs showed an improvement.
• This may be running afoul of Maybe too much noise/undeterminism in our unicode runs atop cachegrind? rustc-perf#1299; so I'm going to chalk this up to noise/undeterminism, rather than a proper regression.

@rustbot label: +perf-regression-triaged

[CAD97]

I want to note that this is a regression in rustdoc output, as https://doc.rust-lang.org/nightly/std/ffi/type.CString.html is now just a link to https://doc.rust-lang.org/nightly/alloc/ffi/struct.CString.html. (This also causes the type to show up as an external type for implementations.)

Also, should this get a tracking issue rather than pointing to this PR?

[joshtriplett]

@CAD97 When we stabilize this, we can switch back from type aliases to re-exports, which should address that.