[PATCH 0/3] string-list: make string_list_sort() reentrant

[PATCH 0/3] string-list: make string_list_sort() reentrant

[René Scharfe]

Use qsort_s() from C11 Annex K to make string_list_sort() safer, in
particular when called from parallel threads.

  compat: add qsort_s()
  add QSORT_S
  string-list: use QSORT_S in string_list_sort()

 Makefile          | 10 ++++++++
 compat/qsort_s.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h | 11 +++++++++
 string-list.c     | 13 ++++-------
 4 files changed, 95 insertions(+), 8 deletions(-)
 create mode 100644 compat/qsort_s.c

-- 
2.11.0

[PATCH 1/3] compat: add qsort_s()

[René Scharfe]

The function qsort_s() was introduced with C11 Annex K; it provides the
ability to pass a context pointer to the comparison function, supports
the convention of using a NULL pointer for an empty array and performs a
few safety checks.

Add an implementation based on compat/qsort.c for platforms that lack a
native standards-compliant qsort_s() (i.e. basically everyone).  It
doesn't perform the full range of possible checks: It uses size_t
instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
because we probably don't have the restricted size type defined.  For
the same reason it returns int instead of errno_t.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 Makefile          | 10 ++++++++
 compat/qsort_s.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |  6 +++++
 3 files changed, 85 insertions(+)
 create mode 100644 compat/qsort_s.c

diff --git a/Makefile b/Makefile
index f53fcc90d..2245fd95d 100644
--- a/Makefile
+++ b/Makefile
@@ -279,6 +279,9 @@ all::
 # is a simplified version of the merge sort used in glibc. This is
 # recommended if Git triggers O(n^2) behavior in your platform's qsort().
 #
+# Define HAVE_QSORT_S if your platform provides a qsort_s() that's
+# compatible with the one described in C11 Annex K.
+#
 # Define UNRELIABLE_FSTAT if your system's fstat does not return the same
 # information on a not yet closed file that lstat would return for the same
 # file after it was closed.
@@ -1423,6 +1426,13 @@ ifdef INTERNAL_QSORT
 	COMPAT_CFLAGS += -DINTERNAL_QSORT
 	COMPAT_OBJS += compat/qsort.o
 endif
+
+ifdef HAVE_QSORT_S
+	COMPAT_CFLAGS += -DHAVE_QSORT_S
+else
+	COMPAT_OBJS += compat/qsort_s.o
+endif
+
 ifdef RUNTIME_PREFIX
 	COMPAT_CFLAGS += -DRUNTIME_PREFIX
 endif
diff --git a/compat/qsort_s.c b/compat/qsort_s.c
new file mode 100644
index 000000000..52d1f0a73
--- /dev/null
+++ b/compat/qsort_s.c
@@ -0,0 +1,69 @@
+#include "../git-compat-util.h"
+
+/*
+ * A merge sort implementation, simplified from the qsort implementation
+ * by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
+ */
+
+static void msort_with_tmp(void *b, size_t n, size_t s,
+			   int (*cmp)(const void *, const void *, void *),
+			   char *t, void *ctx)
+{
+	char *tmp;
+	char *b1, *b2;
+	size_t n1, n2;
+
+	if (n <= 1)
+		return;
+
+	n1 = n / 2;
+	n2 = n - n1;
+	b1 = b;
+	b2 = (char *)b + (n1 * s);
+
+	msort_with_tmp(b1, n1, s, cmp, t, ctx);
+	msort_with_tmp(b2, n2, s, cmp, t, ctx);
+
+	tmp = t;
+
+	while (n1 > 0 && n2 > 0) {
+		if (cmp(b1, b2, ctx) <= 0) {
+			memcpy(tmp, b1, s);
+			tmp += s;
+			b1 += s;
+			--n1;
+		} else {
+			memcpy(tmp, b2, s);
+			tmp += s;
+			b2 += s;
+			--n2;
+		}
+	}
+	if (n1 > 0)
+		memcpy(tmp, b1, n1 * s);
+	memcpy(b, t, (n - n2) * s);
+}
+
+int git_qsort_s(void *b, size_t n, size_t s,
+		int (*cmp)(const void *, const void *, void *), void *ctx)
+{
+	const size_t size = st_mult(n, s);
+	char buf[1024];
+
+	if (!n)
+		return 0;
+	if (!b || !cmp)
+		return -1;
+
+	if (size < sizeof(buf)) {
+		/* The temporary array fits on the small on-stack buffer. */
+		msort_with_tmp(b, n, s, cmp, buf, ctx);
+	} else {
+		/* It's somewhat large, so malloc it.  */
+		char *tmp = xmalloc(size);
+		msort_with_tmp(b, n, s, cmp, tmp, ctx);
+		free(tmp);
+	}
+	return 0;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092..d25f0bd4c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -988,6 +988,12 @@ static inline void sane_qsort(void *base, size_t nmemb, size_t size,
 		qsort(base, nmemb, size, compar);
 }
 
+#ifndef HAVE_QSORT_S
+int git_qsort_s(void *base, size_t nmemb, size_t size,
+		int (*compar)(const void *, const void *, void *), void *ctx);
+#define qsort_s git_qsort_s
+#endif
+
 #ifndef REG_STARTEND
 #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
 #endif
-- 
2.11.0

[PATCH 2/3] add QSORT_S

[René Scharfe]

Add the macro QSORT_S, a convenient wrapper for qsort_s() that infers
the size of the array elements and dies on error.

Basically all possible errors are programming mistakes (passing NULL as
base of a non-empty array, passing NULL as comparison function,
out-of-bounds accesses), so terminating the program should be acceptable
for most callers.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index d25f0bd4c..b707dd880 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -994,6 +994,11 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,
 #define qsort_s git_qsort_s
 #endif
 
+#define QSORT_S(base, n, compar, ctx) do {			\
+	if (qsort_s((base), (n), sizeof(*(base)), compar, ctx))	\
+		die("BUG: qsort_s() failed");			\
+} while (0)
+
 #ifndef REG_STARTEND
 #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
 #endif
-- 
2.11.0

[PATCH 3/3] string-list: use QSORT_S in string_list_sort()

[René Scharfe]

Pass the comparison function to cmp_items() via the context parameter of
qsort_s() instead of using a global variable.  That allows calling
string_list_sort() from multiple parallel threads.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 string-list.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/string-list.c b/string-list.c
index 8c83cac18..45016ad86 100644
--- a/string-list.c
+++ b/string-list.c
@@ -211,21 +211,18 @@ struct string_list_item *string_list_append(struct string_list *list,
 			list->strdup_strings ? xstrdup(string) : (char *)string);
 }
 
-/* Yuck */
-static compare_strings_fn compare_for_qsort;
-
-/* Only call this from inside string_list_sort! */
-static int cmp_items(const void *a, const void *b)
+static int cmp_items(const void *a, const void *b, void *ctx)
 {
+	compare_strings_fn cmp = ctx;
 	const struct string_list_item *one = a;
 	const struct string_list_item *two = b;
-	return compare_for_qsort(one->string, two->string);
+	return cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-	compare_for_qsort = list->cmp ? list->cmp : strcmp;
-	QSORT(list->items, list->nr, cmp_items);
+	QSORT_S(list->items, list->nr, cmp_items,
+		list->cmp ? list->cmp : strcmp);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.11.0

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 01.12.2016 um 17:26 schrieb René Scharfe:
> The function qsort_s() was introduced with C11 Annex K; it provides the
> ability to pass a context pointer to the comparison function, supports
> the convention of using a NULL pointer for an empty array and performs a
> few safety checks.
> 
> Add an implementation based on compat/qsort.c for platforms that lack a
> native standards-compliant qsort_s() (i.e. basically everyone).  It
> doesn't perform the full range of possible checks: It uses size_t
> instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
> because we probably don't have the restricted size type defined.  For
> the same reason it returns int instead of errno_t.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  Makefile          | 10 ++++++++
>  compat/qsort_s.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  git-compat-util.h |  6 +++++
>  3 files changed, 85 insertions(+)
>  create mode 100644 compat/qsort_s.c

And here's the diff for compat/qsort_s.c with copy detection (-C -C):
---
 compat/{qsort.c => qsort_s.c} | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/compat/qsort.c b/compat/qsort_s.c
similarity index 62%
copy from compat/qsort.c
copy to compat/qsort_s.c
index 7d071afb7..52d1f0a73 100644
--- a/compat/qsort.c
+++ b/compat/qsort_s.c
@@ -3,11 +3,12 @@
 /*
  * A merge sort implementation, simplified from the qsort implementation
  * by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
  */
 
 static void msort_with_tmp(void *b, size_t n, size_t s,
-			   int (*cmp)(const void *, const void *),
-			   char *t)
+			   int (*cmp)(const void *, const void *, void *),
+			   char *t, void *ctx)
 {
 	char *tmp;
 	char *b1, *b2;
@@ -21,13 +22,13 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
 	b1 = b;
 	b2 = (char *)b + (n1 * s);
 
-	msort_with_tmp(b1, n1, s, cmp, t);
-	msort_with_tmp(b2, n2, s, cmp, t);
+	msort_with_tmp(b1, n1, s, cmp, t, ctx);
+	msort_with_tmp(b2, n2, s, cmp, t, ctx);
 
 	tmp = t;
 
 	while (n1 > 0 && n2 > 0) {
-		if (cmp(b1, b2) <= 0) {
+		if (cmp(b1, b2, ctx) <= 0) {
 			memcpy(tmp, b1, s);
 			tmp += s;
 			b1 += s;
@@ -44,19 +45,25 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
 	memcpy(b, t, (n - n2) * s);
 }
 
-void git_qsort(void *b, size_t n, size_t s,
-	       int (*cmp)(const void *, const void *))
+int git_qsort_s(void *b, size_t n, size_t s,
+		int (*cmp)(const void *, const void *, void *), void *ctx)
 {
 	const size_t size = st_mult(n, s);
 	char buf[1024];
 
+	if (!n)
+		return 0;
+	if (!b || !cmp)
+		return -1;
+
 	if (size < sizeof(buf)) {
 		/* The temporary array fits on the small on-stack buffer. */
-		msort_with_tmp(b, n, s, cmp, buf);
+		msort_with_tmp(b, n, s, cmp, buf, ctx);
 	} else {
 		/* It's somewhat large, so malloc it.  */
 		char *tmp = xmalloc(size);
-		msort_with_tmp(b, n, s, cmp, tmp);
+		msort_with_tmp(b, n, s, cmp, tmp, ctx);
 		free(tmp);
 	}
+	return 0;
 }

Re: [PATCH 1/3] compat: add qsort_s()

[Jeff King]

On Thu, Dec 01, 2016 at 05:26:43PM +0100, René Scharfe wrote:

> The function qsort_s() was introduced with C11 Annex K; it provides the
> ability to pass a context pointer to the comparison function, supports
> the convention of using a NULL pointer for an empty array and performs a
> few safety checks.
> 
> Add an implementation based on compat/qsort.c for platforms that lack a
> native standards-compliant qsort_s() (i.e. basically everyone).  It
> doesn't perform the full range of possible checks: It uses size_t
> instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
> because we probably don't have the restricted size type defined.  For
> the same reason it returns int instead of errno_t.

Hmm. So it sounds like qsort_r(), but with the NULL-is-empty magic. But
we already are OK without the latter (and can emulate it easily). Would
it make sense to do:

  #if defined(HAVE_QSORT_S)
  /* huzzah, use the system-native qsort_s */

  #elif defined(HAVE_QSORT_R)
  int git_qsort_s(void *b, size_t n, size_t s,
		   int (*cmp)(const void *, const void *, void *), void *ctx)
  {
	if (!n)
		return 0;
	if (!b || !cmp)
		return -1;
	qsort_r(b, n, s, cmp, ctx);
	return 0;
  }

  #else
  /* fallback implementation as your patch does */
  #endif

To make matters more fun, apparently[1] there are multiple variants of
qsort_r with different argument orders. _And_ apparently Microsoft
defines qsort_s, but it's not quite the same thing. But all of that can
be dealt with by having more specific flags (HAVE_GNU_QSORT_R, etc).

It just seems like we should be able to do a better job of using the
system qsort in many cases.

-Peff

[1] https://stackoverflow.com/questions/39560773/different-declarations-of-qsort-r-on-mac-and-linux/39561369

Re: [PATCH 1/3] compat: add qsort_s()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> To make matters more fun, apparently[1] there are multiple variants of
> qsort_r with different argument orders. _And_ apparently Microsoft
> defines qsort_s, but it's not quite the same thing. But all of that can
> be dealt with by having more specific flags (HAVE_GNU_QSORT_R, etc).
>
> It just seems like we should be able to do a better job of using the
> system qsort in many cases.

If we were to go that route, perhaps we shouldn't have HAVE_QSORT_S
so that Microsoft folks won't define it by mistake (instead perhaps
call it HAVE_ISO_QSORT_S or something).

I like your suggestion in general.  The body of git_qsort_s() on
systems without ISO_QSORT_S can do 

 - GNU qsort_r() without any change in the parameters, 

 - Microsoft qsort_s() with parameter reordered, or 

 - Apple/BSD qsort_r() with parameter reordered.

and that would cover the major platforms.

Eh, wait.  BSD and Microsoft have paramters reordered in the
callback comparison function.  I suspect that would not fly very
well.

Re: [PATCH 1/3] compat: add qsort_s()

[Jeff King]

On Thu, Dec 01, 2016 at 12:14:42PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > To make matters more fun, apparently[1] there are multiple variants of
> > qsort_r with different argument orders. _And_ apparently Microsoft
> > defines qsort_s, but it's not quite the same thing. But all of that can
> > be dealt with by having more specific flags (HAVE_GNU_QSORT_R, etc).
> >
> > It just seems like we should be able to do a better job of using the
> > system qsort in many cases.
> 
> If we were to go that route, perhaps we shouldn't have HAVE_QSORT_S
> so that Microsoft folks won't define it by mistake (instead perhaps
> call it HAVE_ISO_QSORT_S or something).
> 
> I like your suggestion in general.  The body of git_qsort_s() on
> systems without ISO_QSORT_S can do 
> 
>  - GNU qsort_r() without any change in the parameters, 
> 
>  - Microsoft qsort_s() with parameter reordered, or 
> 
>  - Apple/BSD qsort_r() with parameter reordered.
> 
> and that would cover the major platforms.
> 
> Eh, wait.  BSD and Microsoft have paramters reordered in the
> callback comparison function.  I suspect that would not fly very
> well.

You can hack around it by passing a wrapper callback that flips the
arguments. Since we have a "void *" data pointer, that would point to a
struct holding the "real" callback and chaining to the original data
pointer.

It does incur the cost of an extra level of indirection for each
comparison, though (not just for each qsort call).

You could do it as zero-cost if you were willing to turn the comparison
function definition into a macro.

-Peff

Re: [PATCH 1/3] compat: add qsort_s()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Eh, wait.  BSD and Microsoft have paramters reordered in the
> callback comparison function.  I suspect that would not fly very
> well.

Hmm.  We could do it like this, which may not be too bad.

#if APPLE_QSORT_R
struct apple_qsort_adapter {
	int (*user_cmp)(const void *, const void *, void *);
	void *user_ctx;
}

static int apple_qsort_adapter_cmp(void *ctx, const void *a, const void *b)
{
	struct apple_qsort_adapter *wrapper_ctx = ctx;
	return wrapper_ctx->user_cmp(a, b, wrapper_ctx->user_ctx);
}
#endif

int git_qsort_s(void *b, size_t n, size_t s,
      	   int (*cmp)(const void *, const void *, void *), void *ctx)
{
	if (!n)
		return 0;
	if (!b || !cmp)
		return -1;
#if GNU_QSORT_R
	qsort_r(b, n, s, cmp, ctx);
#elif APPLE_QSORT_R
	{
		struct appple_qsort_adapter a = { cmp, ctx };
		qsort_r(b, n, s, &a, appple_qsort_adapter_cmp);
	}
#endif
      return 0;
}

Re: [PATCH 1/3] compat: add qsort_s()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 01, 2016 at 12:14:42PM -0800, Junio C Hamano wrote:
>
>> Eh, wait.  BSD and Microsoft have paramters reordered in the
>> callback comparison function.  I suspect that would not fly very
>> well.
>
> You can hack around it by passing a wrapper callback that flips the
> arguments.

Apparently our mails crossed.

Re: [PATCH 1/3] compat: add qsort_s()

[Jeff King]

On Thu, Dec 01, 2016 at 12:22:37PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Eh, wait.  BSD and Microsoft have paramters reordered in the
> > callback comparison function.  I suspect that would not fly very
> > well.
> 
> Hmm.  We could do it like this, which may not be too bad.

Heh. Exactly, but I was too lazy to write it out in my other email. :)

The no-cost version would be more like:

#ifdef APPLE_QSORT_R
#define DECLARE_CMP(func) int func(void *data, const void *va, const void *vb)
#else
#define DECLARE_CMP(func) int func(const void *va, const void *vb, void *data)
#endif

and then:

  DECLARE_CMP(foocmp);
  ...

  DECLARE_CMP(foocmp)
  {
	const struct foo *a = va, *b = vb;
	... etc ...
  }

-Peff

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 01.12.2016 um 21:19 schrieb Jeff King:
> On Thu, Dec 01, 2016 at 12:14:42PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> To make matters more fun, apparently[1] there are multiple variants of
>>> qsort_r with different argument orders. _And_ apparently Microsoft
>>> defines qsort_s, but it's not quite the same thing. But all of that can
>>> be dealt with by having more specific flags (HAVE_GNU_QSORT_R, etc).

AFAIU it went like this:

// FreeBSD 5.0 (2003)
void qsort_r(void *base, size_t nmemb, size_t size,
	void *context,
	int (*compar)(void *context, const void *x, const void *y));

// Microsoft Visual Studio 2005
void qsort_s(void *base, size_t nmemb, size_t size,
	int (*compar)(void *context, const void *x, const void *y),
	void *context);

// glibc 2.8 (2008)
void qsort_r(void *base, size_t nmemb, size_t size,
	int (*compar)(const void *x, const void *y, void *context),
	void *context);

// C11 Annex K (2011)
errno_t qsort_s(void *base, rsize_t nmemb, rsize_t size,
	int (*compar)(const void *x, const void *y, void *context),
	void *context);

>>> It just seems like we should be able to do a better job of using the
>>> system qsort in many cases.

Sure, platform-specific implementations can be shorter.

>> If we were to go that route, perhaps we shouldn't have HAVE_QSORT_S
>> so that Microsoft folks won't define it by mistake (instead perhaps
>> call it HAVE_ISO_QSORT_S or something).

OK.

>> I like your suggestion in general.  The body of git_qsort_s() on
>> systems without ISO_QSORT_S can do
>>
>>  - GNU qsort_r() without any change in the parameters,
>>
>>  - Microsoft qsort_s() with parameter reordered, or
>>
>>  - Apple/BSD qsort_r() with parameter reordered.
>>
>> and that would cover the major platforms.

Yes.

However, for MSys INTERNAL_QSORT is defined for some reason, so the 
platform's qsort(3) is not used there; I guess the same reason applies 
to qsort_s().  If it doesn't then an implementation may want to convert 
a call to the invalid parameter handler (which may show a dialog 
offering to Retry, Continue or Abort) into a non-zero return value.

>> Eh, wait.  BSD and Microsoft have paramters reordered in the
>> callback comparison function.  I suspect that would not fly very
>> well.
>
> You can hack around it by passing a wrapper callback that flips the
> arguments. Since we have a "void *" data pointer, that would point to a
> struct holding the "real" callback and chaining to the original data
> pointer.
>
> It does incur the cost of an extra level of indirection for each
> comparison, though (not just for each qsort call).

Indeed.  We'd need a perf test to measure that overhead before we could 
determine if that's a problem, though.

> You could do it as zero-cost if you were willing to turn the comparison
> function definition into a macro.

Ugh.  That either requires changing the signature of qsort_s() based on 
the underlying native function as well, or using a void pointer to pass 
the comparison function, no?  Let's not do that, at least not without a 
good reason.

René

Re: [PATCH 1/3] compat: add qsort_s()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> You can hack around it by passing a wrapper callback that flips the
>> arguments. Since we have a "void *" data pointer, that would point to a
>> struct holding the "real" callback and chaining to the original data
>> pointer.
>>
>> It does incur the cost of an extra level of indirection for each
>> comparison, though (not just for each qsort call).
>
> Indeed.  We'd need a perf test to measure that overhead before we
> could determine if that's a problem, though.

I agree.  Hopefully it won't be too much cost.

>> You could do it as zero-cost if you were willing to turn the comparison
>> function definition into a macro.
>
> Ugh.  That either requires changing the signature of qsort_s() based
> on the underlying native function as well, or using a void pointer to
> pass the comparison function, no?  Let's not do that, at least not
> without a good reason.

Let's not go there.  It may be zero runtime cost, but the cognitive
cost for people who need to code the comparison callback using the
macro is high.

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 01.12.2016 um 21:22 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Eh, wait.  BSD and Microsoft have paramters reordered in the
>> callback comparison function.  I suspect that would not fly very
>> well.
>
> Hmm.  We could do it like this, which may not be too bad.

It's kinda cool to have a bespoke compatibility layer for major 
platforms, but the more I think about it the less I can answer why we 
would want that.  Safety, reliability and performance can't be good 
reasons -- if our fallback function lacks in these regards then we have 
to improve it in any case.

Text size could be a valid reason, but the full function only adds a bit 
more than 2KB to the unstripped git binary.

The flip side is we'd build an ifdef maze that's harder to read and a 
lot more difficult to test.

What do we get in return for that additional complexity?

> #if APPLE_QSORT_R
> struct apple_qsort_adapter {
> 	int (*user_cmp)(const void *, const void *, void *);
> 	void *user_ctx;
> }
>
> static int apple_qsort_adapter_cmp(void *ctx, const void *a, const void *b)
> {
> 	struct apple_qsort_adapter *wrapper_ctx = ctx;
> 	return wrapper_ctx->user_cmp(a, b, wrapper_ctx->user_ctx);
> }
> #endif
>
> int git_qsort_s(void *b, size_t n, size_t s,
>       	   int (*cmp)(const void *, const void *, void *), void *ctx)
> {
> 	if (!n)
> 		return 0;
> 	if (!b || !cmp)
> 		return -1;
> #if GNU_QSORT_R
> 	qsort_r(b, n, s, cmp, ctx);
> #elif APPLE_QSORT_R
> 	{
> 		struct appple_qsort_adapter a = { cmp, ctx };
> 		qsort_r(b, n, s, &a, appple_qsort_adapter_cmp);
> 	}
> #endif

Nit: The fallback for non-GNU, non-Apple systems is missing here, but 
the idea is illustrated clearly enough.

>       return 0;
> }
>

Re: [PATCH 1/3] compat: add qsort_s()

[Jeff King]

On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:

> It's kinda cool to have a bespoke compatibility layer for major platforms,
> but the more I think about it the less I can answer why we would want that.
> Safety, reliability and performance can't be good reasons -- if our fallback
> function lacks in these regards then we have to improve it in any case.

There may be cases that we don't want to support because of portability
issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
want to replicate that.

I dunno. I am not that opposed to just saying "forget libc qsort, we
always use our internal one which is consistent, performant, and safe".
But when I suggested something similar for our regex library, I seem to
recall there were complaints.

-Peff

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

Am 12.12.2016 um 20:57 schrieb Jeff King:
> On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:
>
>> It's kinda cool to have a bespoke compatibility layer for major platforms,
>> but the more I think about it the less I can answer why we would want that.
>> Safety, reliability and performance can't be good reasons -- if our fallback
>> function lacks in these regards then we have to improve it in any case.
>
> There may be cases that we don't want to support because of portability
> issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
> want to replicate that.

Offloading to GPUs might be a better example; I don't know of a libc 
that does any of that, though (yet).

> I dunno. I am not that opposed to just saying "forget libc qsort, we
> always use our internal one which is consistent, performant, and safe".
> But when I suggested something similar for our regex library, I seem to
> recall there were complaints.

Well, I'm not sure how comparable they are, but perhaps we can avoid 
compat code altogether in this case.  Patch coming in a new thread.

René