Bug 216157 - [exp-run] Standardize on thunk-last qsort_r
Summary: [exp-run] Standardize on thunk-last qsort_r
Status: Closed Overcome By Events
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Port Management Team
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-17 00:57 UTC by Conrad Meyer
Modified: 2018-08-25 21:02 UTC (History)
2 users (show)

See Also:
cem: exp-run?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Conrad Meyer freebsd_committer 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 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 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 2017-01-18 20:33:05 UTC
Do you have links to logs for such ports?  Thanks!
Comment 5 Conrad Meyer freebsd_committer 2017-01-18 21:15:45 UTC
Thanks, this is very helpful.
Comment 6 Conrad Meyer freebsd_committer 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 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 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 :-(.