-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util: detect all possible qsort_r and qsort_s variants #6555
Conversation
Do both the |
src/CMakeLists.txt
Outdated
"" "stdlib.h" GIT_QSORT_R_BSD) | ||
|
||
# new-style BSD and GNU qsort_r() has the 'thunk' parameter as the last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maybe "new style FreeBSD qsort_r" -- I don't know that other BSDs have picked it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use POSIX qsort_r? (reference: https://www.austingroupbugs.net/view.php?id=900 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither NetBSD nor OpenBSD even have qsort_r()
(and they don't have qsort_s()
either), so technically it is more correct to use the term 'FreeBSD', or indeed just 'POSIX' here.
I guess the "old-style BSD" comment should then really be "old-style FreeBSD", but I'm inclined to leave the GIT_QSORT_R_BSD
as it has always been.
src/util/util.c
Outdated
@@ -726,7 +727,9 @@ void git__qsort_r( | |||
qsort_r(els, nel, elsize, &glue, git__qsort_r_glue_cmp); | |||
#elif defined(GIT_QSORT_R_GNU) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses glue when dealing with traditional BSD qsort_r. When both are present, it seems that GIT_QSORT_R_GNU should be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They won't both be present, either the old variant or the new variant will be detected at configure time, but not both. Still, putting the most preferred variants at the top is less ambiguous, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the detection should prefer the POSIX/GNU qsort_r() variant when both are available, because the support of traditional BSD style qsort_r() is done with glues.
Not at the moment, but this is a bug, see below:
The way CMake tests prototypes is by using a template program (in
If this 'fake' definition does not exactly match the declaration in the header, it causes a compile error. For example, for the
I see now that I used the CMake When I fix that, both
From the logic in https://github.com/libgit2/libgit2/blob/main/src/util/util.c#L724, you can see that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
As reported in https://bugs.freebsd.org/271234, recent versions of FreeBSD have adjusted the prototype for qsort_r() to match the POSIX interface. This causes libgit2's CMake configuration check to fail to detect qsort_r(), making it fall back to qsort_s(), which in libgit2 also has an incompatible interface. With recent versions of clang this results in a "incompatible function pointer types" compile error. Summarizing, there are four variations of 'qsort-with-context': * old style BSD qsort_r(), used in FreeBSD 13 and earlier, where the comparison function has the context parameter first * GNU or POSIX qsort_r(), also used in FreeBSD 14 and later, where the comparison function has the context parameter last * C11 qsort_s(), where the comparison function has the context parameter last * Microsoft qsort_s(), where the comparison function has the context parameter first Add explicit detections for all these variants, so they get detected as (in the same order as above): * `GIT_QSORT_R_BSD` * `GIT_QSORT_R_GNU` * `GIT_QSORT_S_C11` * `GIT_QSORT_S_MSC` An additional complication is that on FreeBSD 14 and later, <stdlib.h> uses the C11 _Generic() macro mechanism to automatically select the correct qsort_r() prototype, depending on the caller's comparison function argument. This breaks CMake's check_prototype_definition() functionality, since it tries to redefine the function, and _Generic macro is expanded inline causing a compile error. Work around that problem by putting the function names in parentheses, to prevent the preprocessor from using a macro to replace the function name. Also, in `git__qsort_r()`, change the `#if` order so the variants that do not have to use glue are preferred.
ffa10f4
to
d873966
Compare
Thanks for the PR! I'm not seeing a system sort get selected on our Linux or macOS CI builds, which I would have expected. Is my expectation wrong or do we need to adjust the cmake magic a bit? |
Hmm that's an interesting one, I see for example with the Ubuntu Xenial build (https://github.com/libgit2/libgit2/actions/runs/4918385982/jobs/8796807358?pr=6555):
But when I spin up a local Xenial container, and run the build in there, it works as expected (it only finds the GNU/POSIX variant):
Similar for the macOS build (https://github.com/libgit2/libgit2/actions/runs/4918385982/jobs/8796807907?pr=6555) which shows:
whereas on my own Mac (M1 with macOS 13.3.1 and Xcode 14.3) it correctly detects only the old-style BSD variant:
Can anybody get at the |
Ah, I could pull the container image used for building, and reproduce the problem in there, but it's due to
The test function added by CMake always has an empty body, so the incoming parameters are indeed purposefully unused. The warning should be suppressed, but it's tricky to do this in a platform-independent way, i..e Note also the error on |
2 similar comments
Ah, I could pull the container image used for building, and reproduce the problem in there, but it's due to
The test function added by CMake always has an empty body, so the incoming parameters are indeed purposefully unused. The warning should be suppressed, but it's tricky to do this in a platform-independent way, i..e Note also the error on |
Ah, I could pull the container image used for building, and reproduce the problem in there, but it's due to
The test function added by CMake always has an empty body, so the incoming parameters are indeed purposefully unused. The warning should be suppressed, but it's tricky to do this in a platform-independent way, i..e Note also the error on |
Aha. Thanks for digging deep on this. That makes sense. I don't see an easy way around this. 🙃 Thanks again for the fix. |
Actually I have a fix lined up for this, but I'll put it in another pull request :) |
See #6558. |
Clang 16 has a new error about incompatible function types, which shows up when building devel/libgit2: /wrkdirs/usr/ports/devel/libgit2/work/libgit2-1.5.2/src/util/util.c:731:28: error: incompatible function pointer types passing 'int (void *, const void *, const void *)' to parameter of type 'int (*)(const void *, const void *, void *)' [-Wincompatible-function-pointer-types] qsort_s(els, nel, elsize, git__qsort_r_glue_cmp, &glue); ^~~~~~~~~~~~~~~~~~~~~ /usr/include/stdlib.h:397:11: note: passing argument to parameter here int (*)(const void *, const void *, void *), void *); ^ Clang is indeed right, as the version of qsort_s(3) in FreeBSD 13 and later has the 'void *payload' parameter last: errno_t qsort_s(void *, rsize_t, rsize_t, int (*)(const void *, const void *, void *), void *); This could be fixed by putting the arguments in the right place for qsort_s(3), but it turns out the rabbit hole goes a bit deeper: on 14-CURRENT, libgit2's CMake configuration is not able to detect qsort_r(3), which is actually why it chooses qsort_s(): -- Checking prototype qsort_r for GIT_QSORT_R_BSD -- Checking prototype qsort_r for GIT_QSORT_R_BSD - False -- Checking prototype qsort_r for GIT_QSORT_R_GNU -- Checking prototype qsort_r for GIT_QSORT_R_GNU - False -- Looking for qsort_s -- Looking for qsort_s - found The problem with the GIT_QSORT_R_BSD detection is due to the check in libgit2's src/CMakeLists.txt, where it does: 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 *))" "" "stdlib.h" GIT_QSORT_R_BSD) and CMake attempts to define a function with a similar prototype in its test program, which then fails to compile, at least on 14-CURRENT: /wrkdirs/share/dim/ports/devel/libgit2/work/.build/CMakeFiles/CMakeScratch/TryCompile-tILE28/CheckPrototypeDefinition.c:14:6: error: expected identifier or '(' void qsort_r(void *base, size_t nmemb, size_t size, void *thunk, int (*compar)(void *, const void *, const void *)) { ^ /usr/include/stdlib.h:357:5: note: expanded from macro 'qsort_r' __generic(arg5, int (*)(void *, const void *, const void *), \\ ^ /usr/include/sys/cdefs.h:323:2: note: expanded from macro '__generic' _Generic(expr, t: yes, default: no) ^ This is because in https://cgit.freebsd.org/src/commit/?id=af3c78886fd8d Ed Schouten changed the prototype of qsort_r(3) to match POSIX, using a C11 _Generic macro. When CMake tries to compile its own custom definition of qsort_r, that fails with the above compile error, because the macro gets expanded in place of the function declaration. So the summarized situation is: * On 12.x and 13.x, qsort_r(3) is a plain function, and uses the old comparison function type: 'int (*compar)(void *thunk, const void *elem1, const void *elem2)'. Therefore, CMake detects GIT_QSORT_R_BSD, and libgit2's src/util/util.c uses the correct comparison function type. * On 14.x, qsort_r(3) is a macro, and uses the POSIX comparison function type: 'int (*compar)(const void *elem1, const void *elem2, void *thunk)'. Therefore, CMake fails to detect GIT_QSORT_R_BSD, and detects GIT_QSORT_S instead, and libgit2's src/util/util.c uses an incorrect comparison function type. I submitted libgit2/libgit2#6555 and libgit2/libgit2#6558 upstream to remedy the situation and correctly detect all qsort variants, and these were merged with a few minor changes. This is an adjusted version of the upstream commits that applies on top of libgit-1.5.2. PR: 271234 Approved by: mfechner (maintainer) MFH: 2023Q2
@ethomson do you plan to back-port this also to 1.5 and 1.6 branch? Should I create a new Merge Request for this? |
I added the required patches for FreeBSD for version 1.5 now here: This version is also used for gitaly to make to compilable on FreeBSD 14 which is included here: |
As reported in https://bugs.freebsd.org/271234, recent versions of FreeBSD have adjusted the prototype for
qsort_r()
to match the POSIX interface. This causes libgit2's CMake configuration check to fail to detectqsort_r()
, making it fall back toqsort_s()
, which in libgit2 also has an incompatible interface. With recent versions of clang this results in a "incompatible function pointer types" compile error.Summarizing, there are four variations of 'qsort-with-context':
qsort_r()
, used in FreeBSD 13 and earlier, where the comparison function has the context parameter firstqsort_r()
, also used in FreeBSD 14 and later, where the comparison function has the context parameter lastqsort_s()
, where the comparison function has the context parameter lastqsort_s()
, where the comparison function has the context parameter firstAdd explicit detections for all these variants, so they get detected as (in the same order as above):
GIT_QSORT_R_BSD
GIT_QSORT_R_GNU
GIT_QSORT_S_C11
GIT_QSORT_S_MSC
An additional complication is that on FreeBSD 14 and later,
<stdlib.h>
uses the C11_Generic()
macro mechanism to automatically select the correctqsort_r()
prototype, depending on the caller's comparison function argument. This breaks CMake'scheck_prototype_definition()
functionality, since it tries to redefine the function, and the _Generic macro is expanded inline causing a compile error.Work around that problem by putting the function names in parentheses, to prevent the preprocessor from using a macro to replace the function name.