Bug 216157

Summary: [exp-run] Standardize on thunk-last qsort_r
Product: Ports & Packages Reporter: Conrad Meyer <cem>
Component: Ports FrameworkAssignee: Port Management Team <portmgr>
Status: Closed Overcome By Events    
Severity: Affects Only Me CC: emaste, ports-bugs
Priority: --- Flags: cem: exp-run?
Version: Latest   
Hardware: Any   
OS: Any   

Description Conrad Meyer freebsd_committer freebsd_triage 2017-01-17 00:57:41 UTC 
At Ed's suggestion, let's see how painful switching to thunk-last style qsort_r entirely is.  The kernel and userland consumers in base are pretty few and were trivial to fix.  Ports is more of an open question.

Please run an exp-run of the patch below:
https://people.freebsd.org/~cem/qsort_r.patch
Comment 1 Conrad Meyer freebsd_committer freebsd_triage 2017-01-17 04:02:38 UTC 
Sorry, that patch has a problem.  Please use https://people.freebsd.org/~cem/qsort_r2.patch.
Comment 2 Antoine Brodin freebsd_committer freebsd_triage 2017-01-18 20:28:36 UTC 
This exp-run doesn't look good: some ports build successfully (sometimes with an incompatible pointer type warning) and will only crash at runtime so the exp-run will not catch the problem for them.
Comment 3 Conrad Meyer freebsd_committer freebsd_triage 2017-01-18 20:33:05 UTC 
Do you have links to logs for such ports?  Thanks!
Comment 5 Conrad Meyer freebsd_committer freebsd_triage 2017-01-18 21:15:45 UTC 
Thanks, this is very helpful.
Comment 6 Conrad Meyer freebsd_committer freebsd_triage 2017-02-03 04:27:55 UTC 
Please try an updated patch, https://people.freebsd.org/~cem/qsort_r3.patch .  This restores the backwards-compatibility mode to qsort_r.  Maybe we can consider Ed's proposal of removing that entirely in a future version.
Comment 8 Conrad Meyer freebsd_committer freebsd_triage 2017-02-10 17:54:29 UTC 
(In reply to Antoine Brodin from comment #7)
Thanks!  It looks like just removing the old definition wouldn't be terribly *more* broken than leaving it.

The first is C++ and doesn't see the compatibility definition.  Looks like that is going away in a future release, though: https://github.com/hselasky/midipp/commit/97bcdcf82da22528dbfd17957d8ad6fd448e19eb

For swi-pl, the wrong prototype is detected:
pl-dict.c:137:31: warning: incompatible pointer types passing 'struct sort_r_data *' to parameter of type 'int (*)(const void *, const void *, void *)' [-Wincompatible-pointer-types]
    qsort_r(base, nel, width, &tmp, &sort_r_arg_swap);
                              ^~~~
/usr/include/stdlib.h:326:55: note: expanded from macro 'qsort_r'
            __freebsd11_qsort_r, qsort_r)(base, nmemb, size, A, B)
                                                             ^
/usr/include/stdlib.h:303:12: note: passing argument to parameter here
            int (*)(const void *, const void *, void *), void *);
                  ^

Possibly because sort_r_arg_swap is passed as a pointer to function rather than the function type?  So that one might be fixable from the stdlib header.

Hashcat fails because it compiles in strict C99 mode, no C11 or GCC extensions allowed:

cc -c -O2 -pipe  -fstack-protector -isystem /usr/local/include -fno-strict-aliasing -std=c99 -Iinclude/ -IOpenCL/ -fpic -o obj/shared.NATIVE.SHARED.o src/shared.c
In file included from src/shared.c:156:
include/sort_r.h:145:17: error: expected identifier or '('
    extern void qsort_r(void *base, size_t nel, size_t width, void *thunk,
                ^
/usr/include/stdlib.h:324:2: note: expanded from macro 'qsort_r'
        __builtin_choose_expr( \
        ^
1 error generated.

Same for raptor2, which uses the same copy-pasted sort header.

Well, we could always patch these ports.  But then we could drop the compatibility definition while we're at it.
Comment 9 Conrad Meyer freebsd_committer freebsd_triage 2018-08-25 21:02:19 UTC 
Thanks for the exp-runs.  I'm not going to pursue this further given how painful the change would be.  It's certainly unfortunate that we landed on a different definition of qsort_r than Linux :-(.