[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:

> which is implementation-defined

[[citation needed]] :stuck_out_tongue_winking_eye: 

> was needed to preserve the crashing with SIGSEGV behavior that you might get when func(nullptr) was called with a dynamically linked glibc. Without that, __builtin_trap() will crash with SIGILL instead.

Any function, or just `nan{l|f|f128|f16}`?

Wouldn't you get a segfault anyways as soon as you dereferenced a nullptr parameter?

Either way, that sounds more like a glibc-compatibility decision.

That is slightly different from what `LIBC_ADD_NULL_CHECKS` is documented as doing in https://libc.llvm.org/configure.html though.

> Add nullptr checks in the library’s implementations to some functions for which passing nullptr is undefined behavior.

which makes no mention of compatibility with glibc.  If the actual intent of `LIBC_ADD_NULL_CHECKS` is glibc compat, then I don't think we want to use `LIBC_ADD_NULL_CHECKS` in this PR, and the documentation for `LIBC_ADD_NULL_CHECKS` should be updated to reflect that intent.

> So for the purpose of this PR

The additional temporary variables are ugly; they are not necessary if you have a macro that's simply `if (!macro_param) __builtin_trap()`.

---

Was there an internal discussion that triggered 1896ee38898a73ea9c2894e848884c8999884ab1 that you could cc me on?

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