Rewrite docs for std::ptr

[ecstatic-morse]

This PR attempts to resolve #29371.

Rendered

This is a fairly major rewrite of the std::ptr docs, and deserves a fair bit of scrutiny. It adds links to the GNU libc docs for various instrinsics, adds internal links to types and functions referenced in the docs, adds new, more complex examples for many functions, and introduces a common template for discussing unsafety of functions in std::ptr.

All functions in std::ptr (with the exception of ptr::eq) are unsafe because they either read from or write to a raw pointer. The "Safety" section now informs that the function is unsafe because it dereferences a raw pointer and requires that any pointer to be read by the function points to "a valid vaue of type T".

Additionally, each function imposes some subset of the following conditions on its arguments.

• The pointer points to valid memory.
• The pointer points to initialized memory.
• The pointer is properly aligned.

These requirements are discussed in the "Undefined Behavior" section along with the consequences of using functions that perform bitwise copies without requiring T: Copy. I don't love my new descriptions of the consequences of making such copies. Perhaps the old ones were good enough?

Some issues which still need to be addressed before this is merged:

• The new docs assert that drop_in_place is equivalent to calling read and discarding the value. Is this correct?
• Do write_bytes and swap_nonoverlapping require properly aligned pointers?
• The new example for drop_in_place is a lackluster.
• Should these docs rigorously define what valid memory is? Or should is that the job of the reference? Should we link to the reference?
• Is it correct to require that pointers that will be read from refer to "valid values of type T"?
• I can't imagine ever using {read,write}_volatile with non-Copy types. Should I just link to {read,write} and say that the same issues with non-Copy types apply?
• write_volatile doesn't link back to read_volatile.
• Update docs for the unstable swap_nonoverlapping
• Update docs for the unstable unsafe pointer methods RFC

Looking forward to your feedback.

r? @steveklabnik

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[TimNN]

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:00:46] configure: rust.quiet-tests     := True
---
[00:05:21] tidy error: /checkout/src/libcore/intrinsics.rs:995: line longer than 100 chars
[00:05:21] tidy error: /checkout/src/libcore/intrinsics.rs:1152: unexplained "```ignore" doctest; try one:
[00:05:21]
[00:05:21] * make the test actually pass, by adding necessary imports and declarations, or
[00:05:21] * use "```text", if the code is not Rust code, or
[00:05:21] * use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
[00:05:21] * use "```should_panic", if the code is expected to fail at run time, or
[00:05:21] * use "```no_run", if the code should type-check but not necessary linkable/runnable, or
[00:05:21] * explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

[ecstatic-morse]

The CI build failed because I added an ignored doctest to write_byteswhich demonstrates how to cause UB by overwriting a Box<T> with all zeroes. This example is fairly trivial; I could remove it, but is there a better way to fix this? Perhaps no_run?

[ecstatic-morse]

Sorry for all the failing builds, it took me a while to figure out that I had to do x.py test src/tools/linkchecker.

[steveklabnik]

Ill try to give this a review soon, thanks!

We should also probably have @rust-lang/libs look at it too

[ExpHP]

I don't see the role of having both Safety and Undefined Behavior sections. It seems to me that the convention is to have a single section named Safety.

[ lampam @ 21:52:55 ] (master •58 -7 +7) ~/asd/clone/rust
$ rg '#* Undefined [bB]ehavio(u)?r'
src/libcore/mem.rs
532:/// # Undefined behavior

src/liballoc/raw_vec.rs
154:    /// # Undefined Behavior
171:    /// # Undefined Behavior

$ rg '# Safety'
(far more results)

[ExpHP]

(granted, I often wish the # Safety sections tended to more closely resemble your # Undefined Behavior sections, but that is just my opinion)

[ecstatic-morse]

@steveklabnik mentioned that new docs should make use of Undefined Behavior (I think upper case is correct). All these functions are trivially unsafe because they dereference raw pointers, and the Safety section as I have it doesn't add any new information. Perhaps it could be moved to the module docs and not repeated everywhere?

[ExpHP]

I see. I would favor a change like you just mentioned, but will also wait to see what Steve says. 😉

[steveklabnik]

Originally, I felt like these two cases were pretty distinct, but now, re-thinking about it, I'm not so sure, honestly. In some senses, UB is a subset of safety, and it feels like it's worth calling out separately, but maybe not. dunno.

[scottmcm]

I'm confused; I thought UB was the consequence of not following Safety. Is there a kind of unsafety that doesn't lead to UB?

[ecstatic-morse]

Some background on my thought process:

While writing this, I took the point of view that Safety referred to the language feature (i.e. the subset of rust you must opt into with the unsafe keyword). Some functions (e.g. Vec::set_len) are not obviously unsafe from looking at their signature. The Safety section is probably a good place to discuss this.

In discussing why a function is unsafe, one must communicate what invariants must be maintained to avoid UB. Most of the standard library uses the Safety heading to communicate this. I am now of the opinion that there's never really a reason to have both of these top-level headings (as I do now) since the information is so closely related.

However, for functions whose unsafety is obvious because they take a raw pointer and have names like read, write, and copy, it seemed clearer to name the section discussing its invariants Undefined Behavior. RawVec::from_raw_parts also uses this convention.

[steveklabnik]

@scottmcm "safety" is "memory safety", there are kinds of UB that are not memory safety related.

[ExpHP]

I am bothered by the language "can trigger undefined behavior," since for all intents and purposes, there's no difference from saying that it is undefined behavior.

[ecstatic-morse]

Whoops. I thought I changed this for all functions to "Behavior is undefined if any of the following conditions are violated:", but managed to miss write{,_unaligned,_bytes}.

[steveklabnik]

Other than one tiny nit, the overall formatting and structure, etc, is awesome here! Once someone from libs signs off, and this gets tweaked, i'm happy to

[steveklabnik]

I would drop the "unsafe" bit here and just use "through raw pointers"

[pietroalbini]

Ping from triage! The reviewer asked a signoff from @rust-lang/libs, can someone check this PR?

[pietroalbini]

Ping from triage @rust-lang/libs! Can someone check if the docs here are correct?

[alexcrichton]

I'd personally prefer to fold the safety/undefined behavior sections together and also ensure it's worded something along the lines of "if violated these conditions will cause the function to be unsafe ..." instead of "this function is only unsafe if at least one of these conditions is violated ..." in the sense that these are descriptions of safety but not necessarily 100% exhaustive

[pietroalbini]

Ping from triage @steveklabnik! The author addressed the comments from the libs team.

[steveklabnik]

@bors: r+ rollup

thanks!

[bors]

📌 Commit 827251e has been approved by steveklabnik

[Gankra]

My concerns weren't addressed, and I don't think this documentation should have landed as-is.

[pietroalbini]

Ugh, why did GitHub mark the comments as resolved? :/

[ecstatic-morse]

Yeah, this isn't ready to go yet.

I'm guessing @steveklabnik saw @alexcrichton's comment and not @gankro's concerns.

I'm thinking of ways to address the concerns; it will likely involve refactoring common invariants to the module-level docs. The module docs will then link to the nomicon for a more in-depth discussion on uninitialized memory and pointer aliasing. Stuff which is specific to each function will remain in a list in the function level docs.

Unfortunately, I am currently devoting all of my free time to studying for Code Jam Round 2. I will, in all likelihood, be eliminated this weekend and resume work on this. In the meantime, let me know if you need anything.

[Gankra]

@steveklabnik could you revert this PR, since the author is also fine with not landing it?

[ecstatic-morse]

@gankro, @steveklabnik I've pushed a new commit which adds a module-level definition of what a "valid" pointer is. The list of invariants for each function now links back to this definition.

I put the newly built docs in a gh-pages branch on my fork so that they can be more easily reviewed. I'll keep this up to date as I push changes.

I also removed the erroneous requirement that memory must be initialized if it is to be read from. Instead there is a module-level discussion of how one cannot make any assumptions about the value of uninitialized memory. The example asserts that, even after calling copy on an uninitialized buffer, one cannot assume that the destination buffer is actually equal to the source. I think this effectively communicates the consequences of reading undef memory.

Also, do I need to open a new PR since this accidentally got merged and reverted?

Thanks!

[hanna-kruppe]

What one can and can't do with uninitialized memory is a bit up in the air, but it might very well be much more restrictive than what the current wording implies, so I'd prefer to err on the side of caution in these docs. I don't have specific wording to suggest but here are some things that one might get from the current text that might (likely IMO) not be true when we finally get Unsafe Code Guidelines on this matter:

• Uninitialized memory might change non-deterministically but after you load a value from it, it's some fixed bit pattern for which all the usual rules apply, e.g. after let x = src[0]; expressions like x + x == 2 * x and x >= 0 evaluate to true
• You can branch on its current value (e.g. if src[0] == 0 { ... } else { ... }) and it randomly picks one of the branches
• You can safely output uninitialized memory to a file or the network, it's just completely non-deterministic what bit pattern you get

Note that most of these aren't even true of LLVM's undef, and whatever we get in Rust may very well be much more restrictive than undef (e.g., one proposal is to allow only loading and storing, all arithmetic/comparisons/etc. would be instant UB).

[steveklabnik]

Also, do I need to open a new PR since this accidentally got merged and reverted?

Yes please!

[ecstatic-morse]

@rkruppe I was under the impression that this already was rigorously defined behavior and that I was merely too dumb to find it :) Since defining these semantics is out of scope for this PR, I think it's better to just not mention uninitialized memory at all. I'm assuming whatever choice gets made will permit loads and stores, which is all that is required by the functions in ptr. I'll leave out the module-level discussion, as well as omitting it from the list of invariants for each function.

Also, is there an issue or an internals thread where this is being discussed? I couldn't find one, and all that the nomicon says is that "attempting to interpret [uninitialized] memory as any type will cause undefined behavior".

Edit: here it is

[ecstatic-morse]

New PR opened.