Bug 231256 - [exp-run] Change qsort_r(3) to match glibc
Summary: [exp-run] Change qsort_r(3) to match glibc
Status: New
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: 2018-09-09 06:17 UTC by Ed Schouten
Modified: 2018-12-21 16:38 UTC (History)
2 users (show)

See Also:


Attachments
Patch for fixing up qsort_r() (16.37 KB, patch)
2018-09-10 18:18 UTC, Ed Schouten
no flags Details | Diff
Crude patch to keep raptor2 buildable? (1.32 KB, patch)
2018-09-15 09:53 UTC, Ed Schouten
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Schouten freebsd_committer 2018-09-09 06:17:05 UTC
Hi there,

I'm working on a patch for libc & libkern to change qsort_r(3)'s prototype to match that of glibc. By doing this, the POSIX working group can go ahead with standardising qsort_r(3). I would therefore like to request an exp-run with the following patch applied to the base system (patch only tested on HEAD):

https://reviews.freebsd.org/D17083
https://reviews.freebsd.org/D17083?download=true <- raw diff

Thanks in advance!
Comment 1 Antoine Brodin freebsd_committer 2018-09-09 06:18:39 UTC
This was rejected before, why try to do it again?
Comment 2 Ed Schouten freebsd_committer 2018-09-09 08:23:52 UTC
Hi Antoine,

d'oh! I forgot all about the previous exp-runs cem@ requested. I just commented on D17083 to refer to those.

Back then the main issue was that we couldn't reliably detect bad calls to qsort_r(),
but in the meantime Clang's implementation of _Generic() seems to have improved. For example, I am no longer able to reproduce the issue I demonstrated in https://reviews.freebsd.org/D9169#189476.

Because of this, I think it's worth revisiting this once more. Thanks!
Comment 3 Antoine Brodin freebsd_committer 2018-09-10 17:54:42 UTC
Can you provide a patch that can be applied?
Comment 4 Ed Schouten freebsd_committer 2018-09-10 18:18:49 UTC
Created attachment 197013 [details]
Patch for fixing up qsort_r()

Ah, that's annoying. The patch that I linked (from Phabricator) does not deal with renames/additions in such a way that it can be applied easily. :-/

I've just attached a patch that I've created with 'svn diff --patch-compatible'. I was able to apply it using 'svn patch'. Hope that helps!
Comment 5 Antoine Brodin freebsd_committer 2018-09-13 06:40:17 UTC
(In reply to Ed Schouten from comment #2)
At least 1 port builds successfully despite the incorrect call: devel/pecl-qb

Also there is something strange with at least ruby:

checking for qsort_r... yes
checking whether qsort_r is GNU version... no
checking whether qsort_r is BSD version... no

Is this normal?
Comment 6 Ed Schouten freebsd_committer 2018-09-13 07:19:51 UTC
Hi Antoine,

I just looked at those ports and observed the following:

- pecl-qb: This package calls qsort_r(), explicitly casting all parameters to 'void *'. This makes it impossible to derive anything useful. Considering that this package doesn't use Autoconf for picking the right flavor, let's patch this to take __FreeBSD_version into account.

- Ruby: I filed a pull request for that: https://github.com/ruby/ruby/pull/1954 Let's import that patch once upstream merges it.

Be sure to let me know if there are any other ports that behave out of the ordinary. Thanks!
Comment 7 commit-hook freebsd_committer 2018-09-13 19:26:50 UTC
A commit references this bug:

Author: ed
Date: Thu Sep 13 19:26:18 UTC 2018
New revision: 479692
URL: https://svnweb.freebsd.org/changeset/ports/479692

Log:
  lang/ruby2[345]: Improve qsort_r() detection

  This change merges the following upstream pull request into the Ruby
  interpreter ports:

  	https://github.com/ruby/ruby/pull/1954

  Adding this patch to these ports will ensure that once we patch up
  qsort_r() to be compatible with glibc, Ruby will automatically pick them
  up. Ruby should also build fine without this patch, but this will cause
  it to use its own implementation, which blows up the binary size
  slightly.

  Poudriere runs seem to pass for 11.x amd64. Logs indicate that this
  doesn't negatively affect the existing qsort_r() detection:

  	checking whether qsort_r is GNU version... no
  	checking whether qsort_r is BSD version... yes

  PR:		231256
  Approved by:	sunpoet
  Differential Revision:	https://reviews.freebsd.org/D17157

Changes:
  head/lang/ruby23/files/patch-configure.in
  head/lang/ruby24/files/patch-configure.in
  head/lang/ruby25/files/patch-configure.ac
Comment 9 Ed Schouten freebsd_committer 2018-09-15 08:25:59 UTC
Hi Antoine,

Thanks for the list! The exp-run has finished, I presume? Assuming there are also one or two breakages in the remaining 700 ports, it means around a dozen ports are thus affected. None of them seem to be very high-profile. Not too bad.

I checked the sources of the ports you listed below. Unlike Ruby, none of these seem to use some kind of auto-detection scheme for qsort_r(). They should all be patched up to use __FreeBSD_version after the fact. I'll make sure to send out code reviews/file bugs with patches after D17083 lands.
Comment 10 Antoine Brodin freebsd_committer 2018-09-15 08:30:23 UTC
(In reply to Ed Schouten from comment #9)
The reviews/file bugs with patches have to happen before, not after.
Comment 11 Ed Schouten freebsd_committer 2018-09-15 08:41:22 UTC
How may that be done if the value of __FreeBSD_version is not yet known?
Comment 12 Antoine Brodin freebsd_committer 2018-09-15 08:56:41 UTC
(In reply to Ed Schouten from comment #9)
textproc/raptor2 is kind of high profile, it causes 537 skipped.
Comment 13 Ed Schouten freebsd_committer 2018-09-15 09:53:28 UTC
Created attachment 197107 [details]
Crude patch to keep raptor2 buildable?

Hi Antoine,

With that number of reverse dependencies it would probably make sense to at least provide some workaround for that specific port. Would it be acceptable to add a patch like this to raptor2 and clean it up after the src changes land?