Paths

Table of Contentst

Alter the prototype of qsort_r(3) to match glibc.
AcceptedPublic
Actions

Authored by ed on Sep 8 2018, 10:20 PM.

Details

Reviewers

delphij
kib
cem
jilles
imp
hselasky
pfg

Group Reviewers

manpages

Summary

The Austin Group is considering standardizing qsort_r(3) for the
upcoming revision of the standard. Unfortunately, Linux/glibc-based
systems provide a copy of this function that has a different prototype
than ours. Let's switch over to the same prototype that they are using.
There is no need to remain incompatible. Symbol versioning is used to
keep old binaries working.

My idea would be to commit this change not long after we unfreeze HEAD
after branching stable/12. In other words, this change will only become
part of FreeBSD 13.0.

Test Plan

Run a 'make universe'.
Going to ask the Ports folks to do an exp-run.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 19470
Build 19063: arc lint + arc unit

Event Timeline

ed created this revision.Sep 8 2018, 10:20 PM

Herald added a reviewer: manpages. · View Herald TranscriptSep 8 2018, 10:20 PM

Herald added subscribers: emaste, imp. · View Herald Transcript

Harbormaster completed remote builds in B19467: Diff 47823.Sep 8 2018, 10:20 PM

This is likely to cause huge havoc in the ports tree: we created the API and Apple adopted it. Later glibc and NetBSD adopted their own variant.

I am not completely against the change though: perhaps if we had some compatibility flag to re-enable the original behavior, something like #ifdef LEGACY_BSD_QSORT_R, it would be easier to update the ports.

Do we need to change kernel qsort_r() ?

Can they just adopt the FreeBSD variant ? Anyway, I hope that compilation with unmatched implementation fails loudly. In fact, I do not think it is possible to decide about the change before the exp-run is performed.

lib/libc/stdlib/Symbol.map
123	If committing after 13 is started. this should go into FBSD_1.6 version.
lib/libc/stdlib/qsort.c
214	Why not put this into qsort_r_compat.c ?

In D17083#364265, @pfg wrote:

This is likely to cause huge havoc in the ports tree: we created the API and Apple adopted it. Later glibc and NetBSD adopted their own variant.

Hmm ... the change merely changes the order of the arguments so it may go undetected in the exp-run (?).

I am not completely against the change though: perhaps if we had some compatibility flag to re-enable the original behavior, something like #ifdef LEGACY_BSD_QSORT_R, it would be easier to update the ports.

This needs more though:: I am afraid that adding a new flag for legacy code may not be enough, Perhaps checking the POSIX version makes more sense but I don't have a solution

It's a fair point you two are making: with the change I sent out, code that calls qsort_r() in the existing way will compile without any warnings/errors. This is why I've just changed my patch to add some logic to stdlib.h and libkern.h to detect bad calls, similar to how we dealt with the dirname() / basename() transition (libgen.h). Calls to qsort_r() with the comparator as the last argument will fail explictly.

Random notes:

I will of course bump __FreeBSD_version to reflect this change.
Maybe I can already make a similar change to stdlib.h and libkern.h that explicitly causes 'new' calls to qsort_r() to fail and MFC that?

lib/libc/stdlib/Symbol.map
123	Noted! Will leave it in 1.5 in this diff for the time being, as 1.6 is not valid yet.

Add logic to explictly make old calls to qsort_r(3) fail.

Harbormaster completed remote builds in B19470: Diff 47827.Sep 9 2018, 6:03 AM

It looks like I'm quite the forgetful person. In D9169, cem@ tried to solve the same thing. Eventually he abandoned the idea, because we weren't able to accurately implement the __generic() matching to filter out bad calls.

It turns out that times have changed. If I'm using recent versions of Clang, I am no longer able to reproduce those issues. The example I gave in https://reviews.freebsd.org/D9169#189476 now matches _Generic() properly. This is why I think we should simply give this another try. Adding people who were involved in the original discussion once again.

ed added reviewers: cem, jilles.Sep 9 2018, 8:14 AM

+1 to the general idea from me, and thanks Ed for taking on the work. I will review the patch later.

One thing that came up in exp-runs last time was that lots of ports have some sort of hardcoded #ifdef __FreeBSD__ detection for qsort_r argument ordering.

Additionally, some of those ports are C++ and so the __generic detection at compile time did not detect misuse. We only noticed when (some of) their build-time self-tests segfaulted, IIRC. Can we use an additional mechanism (#if defined(__cplusplus)) to detect misuse at compile time for C++ compilation units as well? I'm worried that without this, we will break C++ ports in a way that is only noticed at runtime.

Edit: Would it be possible to hide the C definition from C++ compilers in the header, and ifdef cplusplus only define a static inline C++ function with stronger type checking that wraps the C function? I am not familiar with the extended C++ feature set.

Apologies if these concerns have been addressed, I have not looked at the patch yet and only skimmed the description and earlier remarks.

Hi Conrad,

In D17083#364441, @cem wrote:

Additionally, some of those ports are C++ and so the __generic detection at compile time did not detect misuse. We only noticed when (some of) their build-time self-tests segfaulted, IIRC. Can we use an additional mechanism (#if defined(__cplusplus)) to detect misuse at compile time for C++ compilation units as well? I'm worried that without this, we will break C++ ports in a way that is only noticed at runtime.

I did some tests, but C++ is something we don't need to worry about, right? C++ implicit pointer conversion rules are different from C. A function pointer doesn't implicitly convert to a void pointer. Consider this piece of code, assuming for the BSD qsort_r() prototype:

#include <stdlib.h>

int cmp(void *thunk, const void *a, const void *b) { return 0; }

int main() {
  int numbers[10];
  qsort_r(numbers, 10, sizeof(int), nullptr, cmp);
}

It fails to build with Clang on a system with a glibc-style qsort_r() function as follows:

bla.cc:7:3: error: no matching function for call to 'qsort_r'
  qsort_r(numbers, 10, sizeof(int), nullptr, cmp);
  ^~~~~~~

And with GCC:

bla.cc: In function ‘int main()’:
bla.cc:7:49: error: invalid conversion from ‘int (*)(void*, const void*, const void*)’ to ‘void*’ [-fpermissive]
   qsort_r(numbers, 10, sizeof(int), nullptr, cmp);
                                                 ^

(Edit: Though unrelated to the discussion at hand, why would C++ code use qsort_r()?)

In D17083#364500, @ed wrote:

I did some tests, but C++ is something we don't need to worry about, right?

Well, I hope you're right :-).

(Edit: Though unrelated to the discussion at hand, why would C++ code use qsort_r()?)

Beats me, man. I don't write C++. ;-)

(Probably, some C++ projects started life as C codebases and switched compilers to add a few features for free. And then never wholesale converted C++ stdlib.)

Edit: I might just be misremembering. Maybe it was C ports that hardcode -std=c99 or c89 and therefore don't get __generic().

ed mentioned this in D17157: lang/ruby2[345]: Improve qsort_r() detection.Sep 13 2018, 7:17 PM

Antoine just gave me the results for an exp-run with this change applied: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231256

Summary: about a dozen ports break. None of them seem to be incredibly high-profile. They also don't contain any logic to reliably detect the flavour at configure/compile-time. I think I want to go ahead with pushing this through as follows:

Add FBSD_1.6 to lib/libc/Versions.def and commit that.
Commit all changes to everything except sys/ to transition qsort_r(3).
Commit all changes to sys/ to transition qsort_r(9).
Bump __FreeBSD_version.
Send patches to maintainers of affected ports.

Any approvals/objections?

In D17083#366231, @ed wrote:

Antoine just gave me the results for an exp-run with this change applied: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231256

Summary: about a dozen ports break. None of them seem to be incredibly high-profile. They also don't contain any logic to reliably detect the flavour at configure/compile-time. I think I want to go ahead with pushing this through as follows:

Add FBSD_1.6 to lib/libc/Versions.def and commit that.

Commit all changes to everything except sys/ to transition qsort_r(3).

Commit all changes to sys/ to transition qsort_r(9).

Bump __FreeBSD_version.

Send patches to maintainers of affected ports.

Any approvals/objections?

You cannot do this until the freeze ends, anyway. And, should we rush now, or wait until POSIX says the final word ?

pfg resigned from this revision.Dec 21 2018, 4:20 PM

Is FBSD_1.5 -> 1.6 still needed? Otherwise, LGTM.

lib/libc/stdlib/qsort_r.c
12–14	Is qsort_b no longer needed? What's this change for?

This revision is now accepted and ready to land.Dec 21 2018, 4:28 PM

Looks good to me (except please do use FBSD_1.6 version).

I'm less certain with the qsort_b part: now we have two different compare routine prototypes and it could be confusing for developers.

lib/libc/stdlib/qsort_r.c
12–14	@cem It looks like it was moved and therefore no longer needed in this file (it's in qsort_r_compat.c, and qsort_b was implemented with the new __qsort_r_compat rountine, and qsort_b API was not changed). Because the prototype difference of the compare function, it can not be implemented with the new qsort_r API. I think we should have a new qsort_b API (the inconsistency with qsort_r would be confusing for developers), but this change will not prevent us from doing it in the future, should we choose to fix it and can be taken care of at a later time.