[libc-commits] [libc] [libc] Expand usage of libc null checks. (PR #116262)

[Nick Desaulniers via libc-commits]

================
@@ -17,6 +18,8 @@ namespace LIBC_NAMESPACE_DECL {
 
 // TODO: Look at performance benefits of comparing words.
 LLVM_LIBC_FUNCTION(void *, memchr, (const void *src, int c, size_t n)) {
+  const unsigned char *src_cpy = (const unsigned char *)src;
+  LIBC_CRASH_ON_NULLPTR(src_cpy);
----------------
nickdesaulniers wrote:

So this pattern of having to create a copy of the parameter with a cast is ugly. I looked into why it's necessary and found 1896ee38898a73ea9c2894e848884c8999884ab1 / #106468.

While the addition of `volatile` to get the compiler to retain the actual dereference was clever, it's still UB to deref null. `-fno-delete-null-ptr-checks` is the flag for clang and gcc to not elide these, which we might want to set for `LIBC_ADD_NULL_CHECKS` just to be paranoid, but...

Instead, the `__builtin_trap` should be sufficient to halt execution, which I think is sufficient to satisfy the intent of LIBC_CRASH_ON_NULLPTR.  I think `LIBC_CRASH_ON_NULLPTR` could be updated and renamed to more precisely describe what it's doing:

1. remove `crashing` and `crash` from `LIBC_CRASH_ON_NULLPTR` (and the comment about it) (while retaining `__builtin_trap`).
2. Rename `LIBC_CRASH_ON_NULLPTR` to `LIBC_TRAP_ON_NULLPTR`.

Do you mind doing those two first, in two distinct PRs? Then this PR can be rebased on those two.

cc @michaelrj-google @lntue 

@michaelrj-google mentions we'll need to triple check which parameters we add `LIBC_CRASH_ON_NULLPTR` to in this PR, since for some functions, nullptr is valid input.  The non-standard nullability attributes come to mind. Perhaps they might serve as good documentation.  https://clang.llvm.org/docs/AttributeReference.html#nullability-attributes

> Note that, unlike the declaration attribute nonnull, the presence of _Nonnull does not imply that passing null is undefined behavior: fetch is free to consider null undefined behavior or (perhaps for backward-compatibility reasons) defensively handle null.

https://github.com/llvm/llvm-project/pull/116262