Bug 271234 - devel/libgit2: qsort_r/qsort_s issues on 14-CURRENT
Summary: devel/libgit2: qsort_r/qsort_s issues on 14-CURRENT
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Matthias Fechner
URL:
Keywords:
Depends on:
Blocks: 271047
  Show dependency treegraph
 
Reported: 2023-05-03 20:11 UTC by Dimitry Andric
Modified: 2023-05-14 19:29 UTC (History)
6 users (show)

See Also:
bugzilla: maintainer-feedback? (mfechner)


Attachments
devel/libgit2: fix build with clang 16 (10.09 KB, patch)
2023-05-08 19:04 UTC, Dimitry Andric
no flags Details | Diff
devel/libgit2: fix build with clang 16 (9.48 KB, patch)
2023-05-14 12:34 UTC, Dimitry Andric
mfechner: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitry Andric freebsd_committer freebsd_triage 2023-05-03 20:11:58 UTC
During an exp-run for llvm 16 (see bug 271047), it turned out that devel/libgit2 failed to build with clang 16:

  /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.

Possible fixes include:

* Adding __FreeBSD_version check, although base commit af3c78886fd8d did not bump the version
* Adding additional CMake logic to correctly work around the _Generic macro definition in 14.x's stdlib.h
* Fixing CMake's check_prototype_definition() <https://cmake.org/cmake/help/latest/module/CheckPrototypeDefinition.html> to put parentheses around the checked function names
* ... haven't thought or more possible fixes here, suggestions welcome

Preferably, any fix should be upstreamable.
Comment 1 Dimitry Andric freebsd_committer freebsd_triage 2023-05-03 21:21:14 UTC
Submitted pull request upstream:
https://github.com/libgit2/libgit2/pull/6555
Comment 2 Dimitry Andric freebsd_committer freebsd_triage 2023-05-08 19:04:57 UTC
Created attachment 242064 [details]
devel/libgit2: fix build with clang 16

Adjusted https://github.com/libgit2/libgit2/pull/6555 to apply on top of libgit-1.5.2.
Comment 3 Matthias Fechner freebsd_committer freebsd_triage 2023-05-08 20:37:40 UTC
(In reply to Dimitry Andric from comment #2)
Thanks a lot for taking care of this!
If a finished solution is accepted upstream we can directly include it or wait for a new release, both options are find for me.
Comment 4 Dimitry Andric freebsd_committer freebsd_triage 2023-05-14 12:34:15 UTC
Created attachment 242162 [details]
devel/libgit2: fix build with clang 16

Upstream merged my original pull request, but then another problem turned up which was addressed in https://github.com/libgit2/libgit2/pull/6558, and eventually upstream landed the following commits:

* https://github.com/libgit2/libgit2/commit/d873966fd (util: detect all possible qsort_r and qsort_s variants)
* https://github.com/libgit2/libgit2/commit/3d9cb5e0c (Work around -Werror problems when detecting qsort variants)
* https://github.com/libgit2/libgit2/commit/767a9a733 (cmake: simplify QSORT names)
* https://github.com/libgit2/libgit2/commit/333573857 (cmake: refactor `check_prototype_definition`)

This new patch combines all of the above, with some adjustments for libgit-1.5.2.
Comment 5 Matthias Fechner freebsd_committer freebsd_triage 2023-05-14 17:51:03 UTC
Thanks a lot, please feel free to commit it!
Comment 6 commit-hook freebsd_committer freebsd_triage 2023-05-14 19:26:04 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=a62f1b5796b641bb502b33f7f073238a49dc4d0c

commit a62f1b5796b641bb502b33f7f073238a49dc4d0c
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2023-05-14 12:26:37 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2023-05-14 19:23:31 +0000

    devel/libgit2: fix build with clang 16

    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 https://github.com/libgit2/libgit2/pull/6555 and
    https://github.com/libgit2/libgit2/pull/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

 devel/libgit2/Makefile                        |   1 +
 devel/libgit2/files/patch-github-pr6555 (new) | 145 ++++++++++++++++++++++++++
 2 files changed, 146 insertions(+)