Details

Reviewers

kib
cem
jilles
imp
hselasky
pauamma_gundo.com
ed
pfg

Group Reviewers

manpages

Commits

rGaf3c78886fd8: Alter the prototype of qsort_r(3) to match POSIX, which adopted the

Summary

Unfortunately, the glibc maintainers, despite knowing the existence of the FreeBSD qsort_r(3) interface in 2004 and refused to add the same interface to glibc based on grounds of the lack of standardization and portability concerns, has decided it was a good idea to introduce their own qsort_r(3) interface in 2007 as a GNU extension with a slightly different and incompatible interface.

With the adoption of their interface as POSIX standard, let's switch to the same prototype, there is no need to remain incompatible.

C++ and C applications written for the historical FreeBSD interface get source level compatibility when building in C++ mode, or when building with a C compiler with C11 generics support, provided that the caller passes a fifth parameter of qsort_r() that exactly matches the historical FreeBSD comparator function pointer type and does not redefine the historical qsort_r(3) prototype in their source code.

Symbol versioning is used to keep old binaries working.

Test Plan

Run a 'make universe'.
portmgr@ did an exp-run (PR 266227).

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

delphij added inline comments.Dec 21 2018, 7:21 PM

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.

Ping?

jilles mentioned this in D23174: Add qsort_s(3).Jan 17 2020, 7:27 PM

mandree mentioned this in rP564228: sysutils/e2fsprogs: update to new upstream release v1.46.0.Feb 6 2021, 3:42 PM

It looks like glibc qsort_r has been accepted for next POSIX
https://austingroupbugs.net/view.php?id=900

Herald added subscribers: linuxkpi, melifaro, donner. · View Herald TranscriptMay 13 2021, 6:43 PM

rpokala added a subscriber: rpokala.May 13 2021, 6:55 PM

rpokala added inline comments.

lib/libc/stdlib/qsort.3
444	to match POSIX

Does this change introduce any changes how qsort works? You should all know that the simple qsort algorithm can be a dangerous stack and time trap.

--HPS

Does this change introduce any changes how qsort works?

The algorithm/implementation remains unchanged.

The change seems good now that __generic is reliable. Though I didn't closely check all the converted calls...

lib/libc/stdlib/qsort.3
35	Bump date

hselasky accepted this revision.Jun 7 2021, 7:21 AM

kib added inline comments.Jun 7 2021, 4:11 PM

sys/sys/libkern.h
207	I am not sure why do we need this in libkern at all.

pstef added a subscriber: pstef.Aug 15 2021, 3:09 PM

We need to revisit this again for FreeBSD 14

Herald added a subscriber: glebius. · View Herald TranscriptApr 4 2022, 5:32 PM

s/13/14/ can probably made on the fly.

lib/libc/stdlib/qsort.3
441

Rebase.

This revision now requires review to proceed.Sep 5 2022, 5:59 AM

Harbormaster completed remote builds in B47243: Diff 110134.Sep 5 2022, 5:59 AM

I've requested an exp-run at https://bugs.freebsd.org/266227

Modified qsort_b to use new qsort_r instead

Harbormaster completed remote builds in B47253: Diff 110174.Sep 6 2022, 1:13 AM

Indent

Harbormaster completed remote builds in B47254: Diff 110175.Sep 6 2022, 1:15 AM

Restore original implementation of qsort_b (through __qsort_r_compat).

GET_BLOCK_FUNCTION() actually expected the invoke method to take
the pointer to block as its first parameter.

I'll create a test case for qsort_b to avoid this from happening.

Harbormaster completed remote builds in B47255: Diff 110176.Sep 6 2022, 6:12 AM

delphij added a parent revision: D36463: libc: add test case for qsort_b(3).Sep 6 2022, 7:24 AM

Manual page change LGTM. I don't understand any of the rest. (Summary will need s/13/14) when or before committing.)

This revision is now accepted and ready to land.Sep 7 2022, 2:43 PM

delphij mentioned this in D36493: libc: Implement bsort(3) bitonic sorting algorithm..Sep 8 2022, 9:53 PM

pi added a subscriber: pi.Sep 9 2022, 7:47 AM

Modify stdlib.h to use _generic to accept both historical and GNU
qsort_r and try not to break applications written for FreeBSD.

For applications that are written for FreeBSD using the historical
interface, attempt to provide best effort source level compatibility
by offering the historical interface.

This revision now requires review to proceed.Sep 12 2022, 1:57 AM

Harbormaster completed remote builds in B47332: Diff 110454.Sep 12 2022, 1:57 AM

Is it possible to emit a #warning when using the compat version?

In D17083#829556, @emaste wrote:

Is it possible to emit a #warning when using the compat version?

Yes I think we can mark the compat symbol with __attribute__((deprecated(, but I'm not 100% sure how that would go with third party software or GCC, etc., perhaps do it in a later commit? (Ideally I think we should just condition out the compatibility with a new POSIX level).

In D17083#829557, @delphij wrote:

In D17083#829556, @emaste wrote:

Is it possible to emit a #warning when using the compat version?

Yes I think we can mark the compat symbol with __attribute__((deprecated(, but I'm not 100% sure how that would go with third party software or GCC, etc., perhaps do it in a later commit? (Ideally I think we should just condition out the compatibility with a new POSIX level).

I have spoken too early...

It seems that by defining __qsort_r_compat with __attribute__((deprecated("Please convert to use new qsort_r(3)"))), I do get warnings for lib/libc/tests/stdlib/qsort_r_compat_test.c (expected, and clang will correctly ignore it when passed -Wno-deprecated-declarations).

However, the warnings is also issued for lib/libc/tests/stdlib/qsort_r_test.c, even though it should (and did) have selected the new qsort_r. I haven't looked deep into clang to find out if it's easy to fix yet.

Update stdlib.h to provide a compatibility shim for C++ code.

Harbormaster completed remote builds in B47451: Diff 110716.Sep 18 2022, 11:07 PM

delphij commandeered this revision.Sep 19 2022, 5:17 AM

delphij edited reviewers, added: ed; removed: delphij.

delphij marked 7 inline comments as done.Sep 19 2022, 5:59 AM

delphij added inline comments.

lib/libc/stdlib/qsort.3
35	Bump date Will do.
444	I'll change this in a future revision.

pauamma_gundo.com requested changes to this revision.Sep 19 2022, 4:17 PM

pauamma_gundo.com added inline comments.

lib/libc/stdlib/qsort.3
444	Still needed. Or did I misunderstand which "future revision" you meant?

This revision now requires changes to proceed.Sep 19 2022, 4:17 PM

delphij marked 3 inline comments as done.Sep 19 2022, 4:20 PM

delphij added inline comments.

lib/libc/stdlib/qsort.3
444	I meant that I'll amend this to "to match POSIX" before pushing to `main` (I'll need to amend `.Dd` date anyway).

Still applicable: Manual page change LGTM. I don't understand any of the rest. (Summary will need s/13/14/ when or before committing.)

lib/libc/stdlib/qsort.3
444	I meant that I'll amend this to "to match POSIX" before pushing to `main` (I'll need to amend `.Dd` date anyway). OK then. Thanks.

This revision is now accepted and ready to land.Sep 19 2022, 5:05 PM

emaste added inline comments.Sep 19 2022, 5:45 PM

lib/libc/gen/scandir-compat11.c
117	what happens to old sw passing its own comparison routine in dcomp?
144	this should just be `freebsd11_sort_thunk`, no? it's not specific to alphasort?

delphij marked 3 inline comments as done.Sep 19 2022, 11:18 PM

delphij added inline comments.

lib/libc/gen/scandir-compat11.c
117	They are unaffected. The use of `freebsd11_alphasort_thunk` is an implementation detail, `dcomp` itself doesn't do anything with the `thunk` (as seen below, the typical `dcomp`, `freebsd11_alphasort` was not modified).
144	Yes (but it's somewhat beyond the scope of this change). I'll leave this open for now and take a look in the weekend.

Fix build with GCC11 in C++ mode as reported by exp-run.

This revision now requires review to proceed.Sep 22 2022, 8:49 AM

Harbormaster completed remote builds in B47489: Diff 110842.Sep 22 2022, 8:49 AM

No manual page change. Assuming none were needed.

This revision is now accepted and ready to land.Sep 23 2022, 2:17 AM

emaste added inline comments.Sep 23 2022, 2:09 PM

include/stdlib.h
336	Maybe we should call it the "historical FreeBSD qsort_r" or BSD qsort_r? To give some context for what the scope of the change is/was.
lib/libc/gen/scandir-compat11.c
144	Yeah it would be a separate change

Rebase; amended the commit message and comments in stdlib.h with additional
information of backward source compatibility and why we are switching to
the standardized interface.

No functional change; the only code change was to match 45ff071ddcca which
will not affect the code logic.

This revision now requires review to proceed.Sep 26 2022, 7:42 AM

Harbormaster completed remote builds in B47537: Diff 110971.Sep 26 2022, 7:43 AM

delphij retitled this revision from Alter the prototype of qsort_r(3) to match glibc. to Alter the prototype of qsort_r(3) to match POSIX, which adopted the glibc-based interface..Sep 26 2022, 7:45 AM

delphij edited the summary of this revision. (Show Details)

delphij edited the test plan for this revision. (Show Details)

delphij marked 3 inline comments as done.

Can't vouch for consistency with source.

This revision is now accepted and ready to land.Sep 27 2022, 3:06 AM

Closed by commit rGaf3c78886fd8: Alter the prototype of qsort_r(3) to match POSIX, which adopted the (authored by ed, committed by delphij). · Explain WhySep 30 2022, 10:31 PM

This revision was automatically updated to reflect the committed changes.

delphij added a commit: rGaf3c78886fd8: Alter the prototype of qsort_r(3) to match POSIX, which adopted the.

Alter the prototype of qsort_r(3) to match POSIX, which adopted the glibc-based interface.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 111291

include/stdlib.h

lib/libc/gen/scandir-compat11.c

lib/libc/gen/scandir.c

lib/libc/stdlib/Makefile.inc

lib/libc/stdlib/Symbol.map

lib/libc/stdlib/qsort.3

lib/libc/stdlib/qsort.c

lib/libc/stdlib/qsort_r.c

lib/libc/stdlib/qsort_r_compat.c

lib/libc/tests/stdlib/Makefile

lib/libc/tests/stdlib/qsort_r_compat_test.c

lib/libc/tests/stdlib/qsort_r_test.c

lib/libproc/proc_sym.c

sys/compat/linuxkpi/common/src/linux_compat.c

sys/dev/bhnd/nvram/bhnd_nvram_store_subr.c

sys/dev/drm2/drm_linux_list_sort.c

sys/libkern/qsort.c

sys/netgraph/ng_ppp.c

sys/sys/libkern.h

Alter the prototype of qsort_r(3) to match POSIX, which adopted the glibc-based interface.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 111291

include/stdlib.h

lib/libc/gen/scandir-compat11.c

lib/libc/gen/scandir.c

lib/libc/stdlib/Makefile.inc

lib/libc/stdlib/Symbol.map

lib/libc/stdlib/qsort.3

lib/libc/stdlib/qsort.c

lib/libc/stdlib/qsort_r.c

lib/libc/stdlib/qsort_r_compat.c

lib/libc/tests/stdlib/Makefile

lib/libc/tests/stdlib/qsort_r_compat_test.c

lib/libc/tests/stdlib/qsort_r_test.c

lib/libproc/proc_sym.c

sys/compat/linuxkpi/common/src/linux_compat.c

sys/dev/bhnd/nvram/bhnd_nvram_store_subr.c

sys/dev/drm2/drm_linux_list_sort.c

sys/libkern/qsort.c

sys/netgraph/ng_ppp.c

sys/sys/libkern.h

Alter the prototype of qsort_r(3) to match POSIX, which adopted the glibc-based interface.
ClosedPublic
Actions

Revision Contents
Changeset List