[PATCH] git-compat-util: Avoid strcasecmp() being inlined

This is necessary so that read_mailmap() can obtain a pointer to the
function.

Signed-off-by: Sebastian Schuberth <redacted>
---
 git-compat-util.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index be1c494..664305c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -85,6 +85,13 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
+#define __NO_INLINE__ /* do not inline strcasecmp() */
+#include <string.h>
+#ifdef HAVE_STRINGS_H
+#include <strings.h> /* for strcasecmp() */
+#endif
+#undef __NO_INLINE__
+
 #ifdef WIN32 /* Both MinGW and MSVC */
 #define _WIN32_WINNT 0x0502
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */

@@ -99,10 +106,6 @@
 #include <stddef.h>
 #include <stdlib.h>
 #include <stdarg.h>
-#include <string.h>
-#ifdef HAVE_STRINGS_H
-#include <strings.h> /* for strcasecmp() */
-#endif
 #include <errno.h>
 #include <limits.h>
 #ifdef NEEDS_SYS_PARAM_H

-- 
1.8.3.mingw.1.2.g56240b5.dirty

Sebastian Schuberth wrote:

This is necessary so that read_mailmap() can obtain a pointer to the
function.

Hm, what platform has strcasecmp() as an inline function?  Is this
allowed by POSIX?  Even if it isn't, should we perhaps just work
around it by providing our own thin static function wrapper in
mailmap.c?

Curious,
Jonathan

On Wed, Sep 11, 2013 at 11:29:21AM -0700, Jonathan Nieder wrote:

Sebastian Schuberth wrote:

quoted

This is necessary so that read_mailmap() can obtain a pointer to the
function.

Hm, what platform has strcasecmp() as an inline function?  Is this
allowed by POSIX?  Even if it isn't, should we perhaps just work
around it by providing our own thin static function wrapper in
mailmap.c?

Environments can implement library functions as macros or even
intrinsics, but C99 requires that they still allow you to access a
function pointer.  And if my reading of C99 6.7.4 is correct, it should
apply to inlines, too, because you should always be able to take the
address of an inline function (though it is a little subtle).

But that does not mean there are not popular platforms that we do not
have to workaround (and the inline keyword is C99 anyway, so all bets
are off for pre-C99 inline implementations).

I would prefer the static wrapper solution you suggest, though. It
leaves the compiler free to optimize the common case of normal
strcasecmp calls, and only introduces an extra function indirection when
using it as a callback (and even then, if we can inline the strcasecmp,
it still ends up as a single function call). The downside is that it has
to be remembered at each site that uses strcasecmp, but we do not use
pointers to standard library functions very often.

-Peff

On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder [off-list ref] wrote:

quoted

This is necessary so that read_mailmap() can obtain a pointer to the
function.

Hm, what platform has strcasecmp() as an inline function?  Is this
allowed by POSIX?  Even if it isn't, should we perhaps just work
around it by providing our own thin static function wrapper in
mailmap.c?

I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
string.h contains the following code (see [1]):

#ifndef __NO_INLINE__
__CRT_INLINE int __cdecl __MINGW_NOTHROW
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
#else
#define strncasecmp _strnicmp
#endif

[1] http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/string.h#l107

-- 
Sebastian Schuberth

On Wed, Sep 11, 2013 at 09:59:53PM +0200, Sebastian Schuberth wrote:

On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder [off-list ref] wrote:

quoted

quoted

This is necessary so that read_mailmap() can obtain a pointer to the
function.

Hm, what platform has strcasecmp() as an inline function?  Is this
allowed by POSIX?  Even if it isn't, should we perhaps just work
around it by providing our own thin static function wrapper in
mailmap.c?

I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
string.h contains the following code (see [1]):

#ifndef __NO_INLINE__
__CRT_INLINE int __cdecl __MINGW_NOTHROW
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
#else
#define strncasecmp _strnicmp
#endif

What is the error the compiler reports? Can it take the address of other
inline functions? For example, can it compile:

    inline int foo(void) { return 5; }
    extern int bar(int (*cb)(void));
    int call(void) { return bar(foo); }

Just wondering if that is the root of the problem, or if maybe there is
something else subtle going on. Also, does __CRT_INLINE just turn into
"inline", or is there perhaps some other pre-processor magic going on?

-Peff

On Wed, Sep 11, 2013 at 11:41 PM, Jeff King [off-list ref] wrote:

quoted

I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
string.h contains the following code (see [1]):

#ifndef __NO_INLINE__
__CRT_INLINE int __cdecl __MINGW_NOTHROW
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
#else
#define strncasecmp _strnicmp
#endif

What is the error the compiler reports? Can it take the address of other

The error message of GCC 4.8.1 is:

    LINK git-credential-store.exe
libgit.a(mailmap.o): In function `read_mailmap':
C:\mingwGitDevEnv\git/mailmap.c:238: undefined reference to `strcasecmp'
collect2.exe: error: ld returned 1 exit status
make: *** [git-credential-store.exe] Error 1

So it's a linker error, not a compiler error.

inline functions? For example, can it compile:

    inline int foo(void) { return 5; }
    extern int bar(int (*cb)(void));
    int call(void) { return bar(foo); }

I had to modify the example slightly to:

inline int foo(void) { return 5; }
extern int bar(int (*cb)(void)) { return cb(); }
int main(void) { return bar(foo); }

And this compiles.

Just wondering if that is the root of the problem, or if maybe there is
something else subtle going on. Also, does __CRT_INLINE just turn into
"inline", or is there perhaps some other pre-processor magic going on?

This is the function definition from string.h after preprocessing:

extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
  {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}

-- 
Sebastian Schuberth

On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:

quoted

Just wondering if that is the root of the problem, or if maybe there is
something else subtle going on. Also, does __CRT_INLINE just turn into
"inline", or is there perhaps some other pre-processor magic going on?

This is the function definition from string.h after preprocessing:

extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
  {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}

I wonder if GCC has changed it's behaviour to more closely match C99.
Clang as a compatibility article about this sort of issue:

    http://clang.llvm.org/compatibility.html#inline

On Wed, Sep 11, 2013 at 9:16 PM, Jeff King [off-list ref] wrote:

I would prefer the static wrapper solution you suggest, though. It
leaves the compiler free to optimize the common case of normal
strcasecmp calls, and only introduces an extra function indirection when
using it as a callback (and even then, if we can inline the strcasecmp,
it still ends up as a single function call). The downside is that it has
to be remembered at each site that uses strcasecmp, but we do not use
pointers to standard library functions very often.

Is it possible to add a test which fails if wrapper is not used?

-- 
Piotr Krukowiecki