[libc] Temporarily suppress -fsanitize=function for qsort comparator

[leonardchan]

Summary

Recent upstream changes to -fsanitize=function find more instances of function type mismatches. One case is with the comparator passed to this class. Libraries like boringssl will tend to pass comparators that take pointers to varying types while this comparator expects to accept const void pointers. Ideally those tools would pass a function that strictly accepts const void*s to avoid UB, or we'd have something like qsort_r/s.

[michaelrj]

LGTM

[davidben]

libc/src/stdlib/qsort.cpp

50

Since BoringSSL is cited, I'll go ahead and explain. BoringSSL's constraint is that our public API (and OpenSSL's) allows callers to pass pointers that with a particular type signature. Specifically, we have the equivalent of a std::vector<T*> (though stored as void*s because the underlying implementation is type-erased) and callers pass comparators that take int(*)(const T *const *p, const T *const *p).

This is the exact same calling convention as qsort (pointers to the elements of the array), but without the type erasure. The problem is casting the function pointer types is UB. Since it's public API, we can't change that easily. But also typed interfaces are good, so I'm not especially inclined to make it void*-based.

Short of stashing the comparator in a thread-local (which would risk reentrancy problems if someone sorts in their comparator), it is not possible to use this function pointer with qsort. qsort's comparator is missing the extra context parameter of qsort_s and qsort_r. Unfortunately, qsort_s and qsort_r are not reliably available (LLVM libc among the offenders). Where they are available, they're inconsistently defined.

bsearch has the same problem, and we eventually just gave up on the libc and wrote our own. We had not yet given up on the libc for sorting but, with this issue, we now have. So this workaround is not necessary for the latest BoringSSL. At the same time, this is absurd. That we had to implement our own sorting function is a failure of the toolchain to provide usable tools, so have a vote from me that you all implement qsort_r or qsort_s.

In particular, "Ideally those tools would pass a function that strictly accepts const void*s to avoid UB" is not viable on its own. LLVM libc would need to provide a better API first. Instead, we had to take the "give up on the libc" route.

[davidben]

libc/src/stdlib/qsort.cpp

50

(Also no need to cite BoringSSL in the comment anymore, as we've since stopped calling qsort here. :-) )

[leonardchan]

https://reviews.llvm.org/D152322 for qsort_r

libc/src/stdlib/qsort.cpp

void swap(size_t i, size_t j) const {
     uint8_t *elem_j = get(j);
     for (size_t b = 0; b < elem_size; ++b) {
       uint8_t temp = elem_i[b];
       elem_i[b] = elem_j[b];
       elem_j[b] = temp;
     }
   }
 
+#if defined(__clang__)
+  // Recent upstream changes to -fsanitize=function find more instances of
+  // function type mismatches. One case is with the comparator passed to this
+  // class. Libraries like boringssl will tend to pass comparators that take
+  // pointers to varying types while this comparator expects to accept const
+  // void pointers. Ideally those tools would pass a function that strictly
+  // accepts const void*s to avoid UB, or we'd have something like qsort_r/s.
+  [[clang::no_sanitize("function")]]
+#endif
   int elem_compare(size_t i, const uint8_t *other) const {
     // An element must compare equal to itself so we don't need to consult the
     // user provided comparator.
     if (get(i) == other)
       return 0;
     return compare(get(i), other);
   }