util: make the qsort_r check work on macOS#4765
Conversation
|
I found it while looking at the tsort implementation, which has worried Coverity for a loooong time (which I think it's actually safe and Coverity is just paranoid). As a heads-up, I've also closed as Intentional some of the defects there (around git_buf_oom usage, and some of the ones I remember taking a hard look at). |
| } | ||
|
|
||
| #if defined(HAVE_QSORT_S) || (defined(HAVE_QSORT_R) && defined(BSD)) | ||
| #if defined(HAVE_QSORT_S) || (defined(HAVE_QSORT_R) && !defined(__GLIBC__)) |
There was a problem hiding this comment.
I feel a bit uneasy with this change. !defined(__GLIBC__) is much broader than defined(BSD), and I don't really know whether it'll break any weird platforms. Shouldn't we instead just add another check for macOS in particular?
There was a problem hiding this comment.
TL;DR every platform is weird 😉. I think it's doable to make a CHECK_C_COMPILE test in CMake that would detect the parameter ordering, which would make it at least C-library-resistant (that's the alternative I proposed).
AFAICT, here is what I've been able to gather :
- qsort_r only exists in FreeBSD/DragonFly, ie. it's not really
defined(BSD), and percolated through to macOS. - Windows has a
qsort_sversion. - This was helpful comment, as well as this one.
I'll try to see how doable it is to detect all those qsort-ses using compilation checks.
| #if defined(HAVE_QSORT_R) && defined(BSD) | ||
| #if defined(HAVE_QSORT_R) && defined(__GLIBC__) | ||
| qsort_r(els, nel, elsize, cmp, payload); | ||
| #elif defined(HAVE_QSORT_R) |
7e3cffb to
9955328
Compare
|
Rebased, checking the prototype is done at compile-time now. |
| " HAVE_QSORT_R_BSD) | ||
| IF (HAVE_QSORT_R_BSD) | ||
| ADD_DEFINITIONS(-DHAVE_QSORT_R_BSD) | ||
| ENDIF () |
There was a problem hiding this comment.
Just so that I'm certain that I understand - the idea here is that we're redefining qsort_r, which will be a failure if we're redefining it with a different prototype? So this will fail to compile on glibc-based systems which have said different prototype, and will successfully compile (albeit, I should think, with a warning) on BSD based systems, where we're redefining it, though with the same prototype.
This is clever. I wonder if we could make a cmake module for it, though, that does the whole QSORT_R and type-of-qsort_r check?
| " HAVE_QSORT_R_BSD) | ||
| IF (HAVE_QSORT_R_BSD) | ||
| ADD_DEFINITIONS(-DHAVE_QSORT_R_BSD) | ||
| ENDIF () |
There was a problem hiding this comment.
I think this approach is a lot more sensible. than what we currently have. I'd like to propose a few changes, though:
- instead of defining
HAVE_QSORT_R, only ever defineHAVE_QSORT_R_BSDandHAVE_QSORT_R_GNU(and adjust locations using the previous define accordingly) - instead of
CHECK_C_SOURCE_COMPILESand/orCHECK_FUNCTION_EXISTS, we should be usingCHECK_PROTOTYPE_DEFINITION
So, something like this:
CHECK_PROTOTYPE_DEFINITION(qsort_r
"void qsort_r(void *base, size_t nmemb, size_t size, void *thunk, int (*compar)(void *, const void *, const void *))"
"NULL" "stdlib.h" HAVE_QSORT_R_BSD)
CHECK_PROTOTYPE_DEFINITION(qsort_r
"void qsort_r(void *base, size_t nmemb, size_t size, int (*compar)(const void *, const void *, void *), void *arg)"
"NULL" "stdlib.h" HAVE_QSORT_R_GNU)
The only thing is that we'd have to include CheckPrototypeDefinition, which is only available since CMake v3.0. There's nothing speaking against distributing it in our cmake/Modules directory, though.
This performs a compile-check by using CMake support, to differentiate the GNU version from the BSD version of qsort_r. Module taken from 4f252abea5f1d17c60f6ff115c9c44cc0b6f1df6, which I've checked against CMake 2.8.11.
9955328 to
1a9cc18
Compare
|
Rebased, and changed to use the CMake module, which I lifted from this. I've tested 2.8.11, and it's okay with configuring that, but it didn't build though :
|
|
Thanks for the fix, and the research on the supportedness of |
|
On Fri, Aug 24, 2018 at 01:08:32PM -0700, Etienne Samson wrote:
Rebased, and changed to use the CMake module, which I lifted from [this](Kitware/CMake@4f252ab).
Thanks a lot, @tiennou!
|
As it seems, there are 2 versions of
qsort_r(a non-standard function) out there : the BSD version, and the glibc version, and they differ in their parameter ordering (which is quite dangerous because using one for the other will result in runtime failures).Right now, the
defined(BSD)check makes us use the "default" version on macOS, even though it has a BSD implementation available, so the check needs to be weakened. An alternative would be to perform a "BSD-style" compilation check in CMake, and act accordingly.