[libc] don't over include stdlib in the hdr declaring bsearch

[nickdesaulniers]

When building overlay mode with GCC in release mode, glibc's stdlib.h contains
an extern inline declaration of bsearch. This breaks our use of the gnu::alias
function attribute in LLVM_LIBC_FUNCTION with GCC because GCC checks that the
aliasee is defined in the same TU (clang does not).

We're looking at also potentially updating our definition of LLVM_LIBC_FUNCTION
from libc/src/__support/common.h. Upon testing, I was able to get
-Wnonnull-compare diagnostics from GCC in our definition of bsearch because
glibc declares bsearch with the fugly nonnull function attribute.

There's more we can do here though to improve our implementation of bsearch.
7.24.5.1 says:

Pointer arguments on such a call shall still have valid values, as
described in 7.1.4.

We could also use either function attributes or parameter attributes to denote
these should not be null (for users/callers) and perhaps still check for
non-null explicitly under some yet to be discussed hardening configurations in
the future.

Link: #60481
Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-nonnull-function-attribute

[llvmbot]

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

When building overlay mode with GCC in release mode, glibc's stdlib.h contains
an extern inline declaration of bsearch. This breaks our use of the gnu::alias
function attribute in LLVM_LIBC_FUNCTION with GCC because GCC checks that the
aliasee is defined in the same TU (clang does not).

We're looking at also potentially updating our definition of LLVM_LIBC_FUNCTION
from libc/src/__support/common.h. Upon testing, I was able to get
-Wnonnull-compare diagnostics from GCC in our definition of bsearch because
glibc declares bsearch with the fugly nonnull function attribute.

There's more we can do here though to improve our implementation of bsearch.
7.24.5.1 says:

Pointer arguments on such a call shall still have valid values, as
described in 7.1.4.

We could also use either function attributes or parameter attributes to denote
these should not be null (for users/callers) and perhaps still check for
non-null explicitly under some yet to be discussed hardening configurations in
the future.

Fixes: #60481
Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-nonnull-function-attribute

---

Full diff: https://github.com/llvm/llvm-project/pull/89471.diff

1 Files Affected:

• (modified) libc/src/stdlib/bsearch.h (+1-1)

diff --git a/libc/src/stdlib/bsearch.h b/libc/src/stdlib/bsearch.h
index 1de7e051ff6c41..3590198ba55704 100644
--- a/libc/src/stdlib/bsearch.h
+++ b/libc/src/stdlib/bsearch.h
@@ -9,7 +9,7 @@
 #ifndef LLVM_LIBC_SRC_STDLIB_BSEARCH_H
 #define LLVM_LIBC_SRC_STDLIB_BSEARCH_H
 
-#include <stdlib.h>
+#include <stddef.h> // size_t
 
 namespace LIBC_NAMESPACE {
 

[nickdesaulniers]

cc @enh-google

[nickdesaulniers]

Probably worth noting that similarly other issues may arise like this for functions other than bsearch. I did only test ninja ninja libc.src.stdlib.bsearch.

[nickdesaulniers]

yeah wctob has this issue, too. and vprintf, and atof...

[nickdesaulniers]

/android0/llvm-project/libc/src/stdlib/atof.cpp:16:28: error: ‘double __llvm_libc_19_0_0_git::atof(const char*)’ specifies less restrictive attributes than its target ‘double atof(const char*)’: ‘leaf’, ‘nonnull’, ‘nothrow’, ‘pure’ [-Werror=missing-attributes]
   16 | LLVM_LIBC_FUNCTION(double, atof, (const char *str)) {
      |                            ^~~~

/android0/llvm-project/libc/src/wchar/wctob.cpp:17:25: error: ‘int __llvm_libc_19_0_0_git::wctob(wint_t)’ specifies less restrictive attributes than its target ‘int wctob(wint_t)’: ‘leaf’, ‘nothrow’ [-Werror=missing-attributes]
   17 | LLVM_LIBC_FUNCTION(int, wctob, (wint_t c)) {
      |                         ^~~~~

maybe interesting. I'll think harder about it next week.

[enh-google]

interestingly, it looks like bionic and glibc disagree on the nullability? bionic has

void* _Nullable bsearch(const void* _Nonnull __key,
                        const void* _Nullable __base,
                        size_t __nmemb, size_t __size,
                        int (* _Nonnull __comparator)(const void* _Nonnull __lhs, const void* _Nonnull __rhs));

iirc we went with a non-null base, but found too much code that felt that that should be fine as long as nmemb is 0 (and our implementation was fine with that anyway).

[nickdesaulniers]

(changed fixes tag to link tag), since those 3 other issues are more complex to resolve & might necessitate the changes to LLVM_LIBC_FUNCTION I'm testing in #89333 (comment).

[nickdesaulniers]

interestingly, it looks like bionic and glibc disagree on the nullability? bionic has

Something the C standards body could actually help with, instead adding more crap. I was just reading through

• N3222
• N3089

this morning.

I guess I'd better contact my company's WG14 rep...oh wait... 💀 ☠️ 🧟

[enh-google]

interestingly, it looks like bionic and glibc disagree on the nullability? bionic has

Something the C standards body could actually help with, instead adding more crap.

careful what you wish for anyway... they're likely to go with standardizing something you don't want, either because it's too strict and breaks code you need to support, or not strict enough (to not break anyone's code, including code you don't need to support).

#include my usual "i'm not sure one size actually fits all" :-)

[michaelrj-google]

LGTM

[nickdesaulniers]

Filed #89474 against clang.