Page MenuHomeFreeBSD

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

Authored by delphij on Sep 8 2018, 10:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 11, 10:16 PM
Unknown Object (File)
Mon, Mar 11, 8:09 PM
Unknown Object (File)
Sat, Mar 9, 12:51 AM
Unknown Object (File)
Feb 9 2024, 10:18 AM
Unknown Object (File)
Feb 3 2024, 1:27 PM
Unknown Object (File)
Feb 1 2024, 4:27 PM
Unknown Object (File)
Jan 30 2024, 4:06 AM
Unknown Object (File)
Jan 22 2024, 1:54 PM

Details

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 Passed
Unit
No Test Coverage
Build Status
Buildable 47254
Build 44141: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

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

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

sys/sys/libkern.h
207

I am not sure why do we need this in libkern at all.

We need to revisit this again for FreeBSD 14

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

lib/libc/stdlib/qsort.3
441
This revision now requires review to proceed.Sep 5 2022, 5:59 AM

Modified qsort_b to use new qsort_r instead

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.

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

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

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

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

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.

delphij edited reviewers, added: ed; removed: delphij.
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 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 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
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 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.

delphij marked an inline comment as done.

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

No manual page change. Assuming none were needed.

This revision is now accepted and ready to land.Sep 23 2022, 2:17 AM
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
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