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

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

[Sebastian Schuberth]

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

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 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

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

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

Whoa, I didn't think it is even legal for a C library to supply
strcmp() or strcasecmp() that are purely inline you cannot take the
address of.  The "solution" looks a bit too large a hammer that
affects everybody, not just those who have such a set of header
files.

>  
> +#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

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

[Jeff King]

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

> 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?

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

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

[Sebastian Schuberth]

On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder <jrnieder@gmail.com> 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?

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

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

[Jeff King]

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

> On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder <jrnieder@gmail.com> 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?
> 
> 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

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

[Sebastian Schuberth]

On Wed, Sep 11, 2013 at 11:41 PM, Jeff King <peff@peff.net> wrote:

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

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

[John Keeping]

On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:
> > 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

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:
>> > 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

Interesting.  The ways the page suggests as fixes are

 - change it to a "statis inline";
 - remove "inline" from the definition;
 - provide an external (non-inline) def somewhere else;
 - compile with gnu899 dialect.

But the first two are non-starter, and the third one to force
everybody to define an equivalent implementation is nonsense, for a
definition in the standard header file.

I agree with an earlier conclusion that defining our own wrapper
(with an explanation why such a redundant wrapper exists) is the
best course of action at this point, until the system header is
fixed.

 mailmap.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index a7969c4..d36d424 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)
 	string_list_clear_func(&me->namemap, free_mailmap_info);
 }
 
+/*
+ * On some systems, string.h has _only_ inline definition of strcasecmp
+ * without supplying a non-inline implementation anywhere, which is, eh,
+ * "unusual"; we cannot take an address of such a function to store it in
+ * namemap.cmp.  This is here as a workaround---do not assign strcasecmp
+ * directly to namemap.cmp until we know no systems that matter have such
+ * an "unusual" string.h.
+ */
+static int namemap_cmp(const char *a, const char *b)
+{
+	return strcasecmp(a, b);
+}
+
 static void add_mapping(struct string_list *map,
 			char *new_name, char *new_email,
 			char *old_name, char *old_email)
@@ -75,7 +88,7 @@ static void add_mapping(struct string_list *map,
 		item = string_list_insert_at_index(map, index, old_email);
 		me = xcalloc(1, sizeof(struct mailmap_entry));
 		me->namemap.strdup_strings = 1;
-		me->namemap.cmp = strcasecmp;
+		me->namemap.cmp = namemap_cmp;
 		item->util = me;
 	}
 
@@ -237,7 +250,7 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 	int err = 0;
 
 	map->strdup_strings = 1;
-	map->cmp = strcasecmp;
+	map->cmp = namemap_cmp;
 
 	if (!git_mailmap_blob && is_bare_repository())
 		git_mailmap_blob = "HEAD:.mailmap";

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

[Jeff King]

On Thu, Sep 12, 2013 at 08:37:08AM -0700, Junio C Hamano wrote:

> > 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
> 
> Interesting.  The ways the page suggests as fixes are
> 
>  - change it to a "statis inline";
>  - remove "inline" from the definition;
>  - provide an external (non-inline) def somewhere else;
>  - compile with gnu899 dialect.

Right, option 3 seems perfectly reasonable to me, as we must be prepared
to cope with a decision not to inline the function, and there has to be
_some_ linked implementation. But shouldn't libc be providing an
external, linkable strcasecmp in this case?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 12, 2013 at 08:37:08AM -0700, Junio C Hamano wrote:
>
>> > 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
>> 
>> Interesting.  The ways the page suggests as fixes are
>> 
>>  - change it to a "statis inline";
>>  - remove "inline" from the definition;
>>  - provide an external (non-inline) def somewhere else;
>>  - compile with gnu899 dialect.
>
> Right, option 3 seems perfectly reasonable to me, as we must be prepared
> to cope with a decision not to inline the function, and there has to be
> _some_ linked implementation. But shouldn't libc be providing an
> external, linkable strcasecmp in this case?

That is exactly my point when I said that the third one is nonsense
for a definition in the standard header file.

I think we would want something like below.

-- >8 --
Subject: [PATCH] mailmap: work around implementations with pure inline strcasecmp

On some systems, string.h has _only_ inline definition of strcasecmp
without supplying a non-inline implementation anywhere, which is,
eh, "unusual"; we cannot take an address of such a function to store
it in namemap.cmp.  Work it around by introducing our own level of
indirection.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mailmap.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 44614fc..8863e23 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)
 	string_list_clear_func(&me->namemap, free_mailmap_info);
 }
 
+/*
+ * On some systems, string.h has _only_ inline definition of strcasecmp
+ * without supplying a non-inline implementation anywhere, which is, eh,
+ * "unusual"; we cannot take an address of such a function to store it in
+ * namemap.cmp.  This is here as a workaround---do not assign strcasecmp
+ * directly to namemap.cmp until we know no systems that matter have such
+ * an "unusual" string.h.
+ */
+static int namemap_cmp(const char *a, const char *b)
+{
+	return strcasecmp(a, b);
+}
+
 static void add_mapping(struct string_list *map,
 			char *new_name, char *new_email,
 			char *old_name, char *old_email)
@@ -75,7 +88,7 @@ static void add_mapping(struct string_list *map,
 		item = string_list_insert_at_index(map, index, old_email);
 		me = xcalloc(1, sizeof(struct mailmap_entry));
 		me->namemap.strdup_strings = 1;
-		me->namemap.cmp = strcasecmp;
+		me->namemap.cmp = namemap_cmp;
 		item->util = me;
 	}
 
@@ -241,7 +254,7 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 	int err = 0;
 
 	map->strdup_strings = 1;
-	map->cmp = strcasecmp;
+	map->cmp = namemap_cmp;
 
 	if (!git_mailmap_blob && is_bare_repository())
 		git_mailmap_blob = "HEAD:.mailmap";
-- 
1.8.4-485-gec42fe2

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

[Jonathan Nieder]

Junio C Hamano wrote:

> I think we would want something like below.

Looks good to me, but

> -- >8 --
> Subject: [PATCH] mailmap: work around implementations with pure inline strcasecmp
>
> On some systems, string.h has _only_ inline definition of strcasecmp

Please specify which system we are talking about: s/some systems/MinGW 4.0/

[...]
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)
>  	string_list_clear_func(&me->namemap, free_mailmap_info);
>  }
>  
> +/*
> + * On some systems, string.h has _only_ inline definition of strcasecmp

Likewise.

With or without that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Jeff King]

On Thu, Sep 12, 2013 at 11:35:21AM -0700, Junio C Hamano wrote:

> >>  - change it to a "statis inline";
> >>  - remove "inline" from the definition;
> >>  - provide an external (non-inline) def somewhere else;
> >>  - compile with gnu899 dialect.
> >
> > Right, option 3 seems perfectly reasonable to me, as we must be prepared
> > to cope with a decision not to inline the function, and there has to be
> > _some_ linked implementation. But shouldn't libc be providing an
> > external, linkable strcasecmp in this case?
> 
> That is exactly my point when I said that the third one is nonsense
> for a definition in the standard header file.

Yes, but I am saying it is the responsibility of libc. IOW, I am
wondering if this particular mingw environment is simply broken, and if
so, what is the status on the fix?  Could another option be to declare
the environment unworkable and tell people to upgrade?

I am not even sure if we are right to call it broken, but talking to the
mingw people might be a good next step, as they will surely have an
opinion. :)

-Peff

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

[Sebastian Schuberth]

On Thu, Sep 12, 2013 at 8:20 PM, Jeff King <peff@peff.net> wrote:

>> > 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
>>
>> Interesting.  The ways the page suggests as fixes are
>>
>>  - change it to a "statis inline";
>>  - remove "inline" from the definition;
>>  - provide an external (non-inline) def somewhere else;
>>  - compile with gnu899 dialect.
>
> Right, option 3 seems perfectly reasonable to me, as we must be prepared
> to cope with a decision not to inline the function, and there has to be
> _some_ linked implementation. But shouldn't libc be providing an
> external, linkable strcasecmp in this case?

MinGW / GCC is not linking against libc, but against MSVCRT, Visual
Studio's C runtime. And in fact MSVCRT has a non-inline implementation
of a "case-insensitive string comparison for up to the first n
characters"; it just happens to be called "_strnicmp", not
"strncasecmp". Which is why I still think just having a "#define
strncasecmp _strnicmp" is the most elegant solution to the problem.
And that's exactly what defining __NO_INLINE__ does. Granted, defining
__NO_INLINE__ in the scope of string.h will also add a "#define
strcasecmp _stricmp"; but despite it's name, defining __NO_INLINE__
does not imply a performance hit due to functions not being inlined
because it's just the "strncasecmp" wrapper around "_strnicmp" that's
being inlined, not "_strnicmp" itself.

-- 
Sebastian Schuberth

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

[Sebastian Schuberth]

On Thu, Sep 12, 2013 at 8:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

> Looks good to me, but
>
>> -- >8 --
>> Subject: [PATCH] mailmap: work around implementations with pure inline strcasecmp
>>
>> On some systems, string.h has _only_ inline definition of strcasecmp
>
> Please specify which system we are talking about: s/some systems/MinGW 4.0/

I'm not too happy with the wording either. As I see it, even on MinGW
runtime version 4.0 it's not true that "string.h has _only_ inline
definition of strcasecmp"; there's also "#define strncasecmp
_strnicmp" which effectively provides a non-inline definition of
strncasecmp aka _strnicmp.

-- 
Sebastian Schuberth

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> I'm not too happy with the wording either. As I see it, even on MinGW
> runtime version 4.0 it's not true that "string.h has _only_ inline
> definition of strcasecmp"; there's also "#define strncasecmp
> _strnicmp" which effectively provides a non-inline definition of
> strncasecmp aka _strnicmp.

I do not get this part.  Sure, string.h would have definitions of
things other than strcasecmp, such as strncasecmp.  So what?

Does it "effectively" provide a non-inline definition of strcasecmp?

Perhaps the real issue is that the header file does not give an
equivalent "those who want to take the address of strcasecmp will
get the address of _stricmp instead" macro, e.g.

	#define strcasecmp _stricmp

or something?

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

[Jeff King]

On Thu, Sep 12, 2013 at 09:46:51PM +0200, Sebastian Schuberth wrote:

> > Right, option 3 seems perfectly reasonable to me, as we must be prepared
> > to cope with a decision not to inline the function, and there has to be
> > _some_ linked implementation. But shouldn't libc be providing an
> > external, linkable strcasecmp in this case?
> 
> MinGW / GCC is not linking against libc, but against MSVCRT, Visual
> Studio's C runtime. And in fact MSVCRT has a non-inline implementation
> of a "case-insensitive string comparison for up to the first n
> characters"; it just happens to be called "_strnicmp", not
> "strncasecmp". Which is why I still think just having a "#define
> strncasecmp _strnicmp" is the most elegant solution to the problem.
> And that's exactly what defining __NO_INLINE__ does. Granted, defining
> __NO_INLINE__ in the scope of string.h will also add a "#define
> strcasecmp _stricmp"; but despite it's name, defining __NO_INLINE__
> does not imply a performance hit due to functions not being inlined
> because it's just the "strncasecmp" wrapper around "_strnicmp" that's
> being inlined, not "_strnicmp" itself.

Ah, thanks, that explains what is going on. I do think the environment
is probably in violation of C99, but I dug in the mingw history, and it
looks like it has been this way for over 10 years.

So it is probably worth working around, but it would be nice if the
damage could be contained to just the affected platform.

I think there are basically three classes of solution:

  1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other
     environments, who would then not inline and lose performance (but
     since it's a non-standard macro, we don't really know what it will
     do in other places; possibly nothing).

  2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it
     only affects mingw, and we know the meaning of NO_INLINE there.

  3. Try to impact only the uses as a function pointer (e.g., by using
     a wrapper function as suggested in the thread).

Your patch does (1), I believe. Junio's patch does (3), but is a
maintenance burden in that any new callsites will need to remember to do
the same trick.

But your argument (and reading the mingw header, I agree) is that there
is no performance difference at all between (2) and (3). And (2) does
not have the maintenance burden. So it does seem like the right path to
me.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think there are basically three classes of solution:
>
>   1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other
>      environments, who would then not inline and lose performance (but
>      since it's a non-standard macro, we don't really know what it will
>      do in other places; possibly nothing).
>
>   2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it
>      only affects mingw, and we know the meaning of NO_INLINE there.
>
>   3. Try to impact only the uses as a function pointer (e.g., by using
>      a wrapper function as suggested in the thread).
>
> Your patch does (1), I believe. Junio's patch does (3), but is a
> maintenance burden in that any new callsites will need to remember to do
> the same trick.
>
> But your argument (and reading the mingw header, I agree) is that there
> is no performance difference at all between (2) and (3). And (2) does
> not have the maintenance burden. So it does seem like the right path to
> me.

Agreed.  If that #define __NO_INLINE__ does not appear in the common
part of our header files like git-compat-util.h but is limited to
somewhere in compat/, that would be the perfect outcome.

Thanks, both.

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

[Jonathan Nieder]

Sebastian Schuberth wrote:

> And that's exactly what defining __NO_INLINE__ does. Granted, defining
> __NO_INLINE__ in the scope of string.h will also add a "#define
> strcasecmp _stricmp"; but despite it's name, defining __NO_INLINE__
> does not imply a performance hit due to functions not being inlined
> because it's just the "strncasecmp" wrapper around "_strnicmp" that's
> being inlined, not "_strnicmp" itself.

What I don't understand is why the header doesn't use "static inline"
instead of "extern inline".  The former would seem to be better in
every way for this particular use case.

See also <http://www.greenend.org.uk/rjk/tech/inline.html>, section
"GNU C inline rules".

Thanks,
Jonathan

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

[Jonathan Nieder]

Sebastian Schuberth wrote:

> I'm not too happy with the wording either. As I see it, even on MinGW
> runtime version 4.0 it's not true that "string.h has _only_ inline
> definition of strcasecmp"; there's also "#define strncasecmp
> _strnicmp"

I assume you mean "#define strcasecmp _stricmp", which is guarded by
defined(__NO_INLINE__).  I think what Junio meant is that by default
(i.e., in the !defined(__NO_INLINE__) case) string.h uses
__CRT_INLINE, defined as

	extern inline __attribute__((__gnu_inline__))

to suppress the non-inline function definition.

Jonathan

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

[Sebastian Schuberth]

On Thu, Sep 12, 2013 at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> I'm not too happy with the wording either. As I see it, even on MinGW
>> runtime version 4.0 it's not true that "string.h has _only_ inline
>> definition of strcasecmp"; there's also "#define strncasecmp
>> _strnicmp" which effectively provides a non-inline definition of
>> strncasecmp aka _strnicmp.
>
> I do not get this part.  Sure, string.h would have definitions of
> things other than strcasecmp, such as strncasecmp.  So what?

Sorry, I mixed up "strcasecmp" and "strncasecmp".

> Does it "effectively" provide a non-inline definition of strcasecmp?

Yes, if __NO_INLINE__ is defined string.h provides non-inline
definition of both "strcasecmp" and "strncasecmp" by defining them to
"_stricmp" and "_strnicmp" respectively.

> Perhaps the real issue is that the header file does not give an
> equivalent "those who want to take the address of strcasecmp will
> get the address of _stricmp instead" macro, e.g.
>
>         #define strcasecmp _stricmp
>
> or something?

Now it's you who puzzles me, because the header file *does* have
exactly the macro that you suggest.

Anyway, I think Peff's reply to my other mail summed it up nicely. I
will come up with another patch.

-- 
Sebastian Schuberth

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

[Sebastian Schuberth]

On Thu, Sep 12, 2013 at 10:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:
>
>> I think there are basically three classes of solution:
>>
>>   1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other
>>      environments, who would then not inline and lose performance (but
>>      since it's a non-standard macro, we don't really know what it will
>>      do in other places; possibly nothing).
>>
>>   2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it
>>      only affects mingw, and we know the meaning of NO_INLINE there.
>>
>>   3. Try to impact only the uses as a function pointer (e.g., by using
>>      a wrapper function as suggested in the thread).
>>
>> Your patch does (1), I believe. Junio's patch does (3), but is a
>> maintenance burden in that any new callsites will need to remember to do
>> the same trick.

Well, if by "everywhere" in (1) you mean "on all platforms" then
you're right. But my patch does not define __NO_INLINE__ globally, but
only at the time string.h / strings.h is included. Afterwards
__NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not
defined "everywhere".

> Agreed.  If that #define __NO_INLINE__ does not appear in the common
> part of our header files like git-compat-util.h but is limited to
> somewhere in compat/, that would be the perfect outcome.

It's not that easy to move the definition of __NO_INLINE__ into
compat/ because git-compat-util.h includes string.h / strings.h before
anything of compat/. More over, defining __NO_INLINE__ in somewhere in
compat/ would not limit its definition to the string.h / strings.h
headers only. So how about something like this on top of my original
patch:

--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,12 +85,16 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1

+#ifdef __MINGW32__
 #define __NO_INLINE__ /* do not inline strcasecmp() */
+#endif
 #include <string.h>
+#ifdef __MINGW32__
+#undef __NO_INLINE__
+#endif
 #ifdef HAVE_STRINGS_H
 #include <strings.h> /* for strcasecmp() */
 #endif
-#undef __NO_INLINE__

 #ifdef WIN32 /* Both MinGW and MSVC */
 #ifndef _WIN32_WINNT

-- 
Sebastian Schuberth

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On Thu, Sep 12, 2013 at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> I'm not too happy with the wording either. As I see it, even on MinGW
>>> runtime version 4.0 it's not true that "string.h has _only_ inline
>>> definition of strcasecmp"; there's also "#define strncasecmp
>>> _strnicmp" which effectively provides a non-inline definition of
>>> strncasecmp aka _strnicmp.
>>
>> I do not get this part.  Sure, string.h would have definitions of
>> things other than strcasecmp, such as strncasecmp.  So what?
>
> Sorry, I mixed up "strcasecmp" and "strncasecmp".

OK.

>> Does it "effectively" provide a non-inline definition of strcasecmp?
>
> Yes, if __NO_INLINE__ is defined string.h provides non-inline
> definition of both "strcasecmp" and "strncasecmp" by defining them to
> "_stricmp" and "_strnicmp" respectively.
>
>> Perhaps the real issue is that the header file does not give an
>> equivalent "those who want to take the address of strcasecmp will
>> get the address of _stricmp instead" macro, e.g.
>>
>>         #define strcasecmp _stricmp
>>
>> or something?
>
> Now it's you who puzzles me, because the header file *does* have
> exactly the macro that you suggest.

Then why does your platform have problem with the code that takes
the address of strcasecmp and stores it in the variable?  It is not
me, but your platform that is puzzling us.

There is something else going on, like you do not have that #define
"enabled" under some condition, or something silly like that.

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> Well, if by "everywhere" in (1) you mean "on all platforms" then
> you're right. But my patch does not define __NO_INLINE__ globally, but
> only at the time string.h / strings.h is included. Afterwards
> __NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not
> defined "everywhere".

Which means people who do want to see that macro defined will be
broken after that section of the header file which unconditionally
undefs it, right?

That is exactly why that change should not appear in the platform
neutral part of the header file.

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -85,12 +85,16 @@
>  #define _NETBSD_SOURCE 1
>  #define _SGI_SOURCE 1
>
> +#ifdef __MINGW32__
>  #define __NO_INLINE__ /* do not inline strcasecmp() */
> +#endif
>  #include <string.h>
> +#ifdef __MINGW32__
> +#undef __NO_INLINE__
> +#endif

That is certainly better than the unconditional one, but I wonder if
it is an option to add compat/mingw/string.h without doing the
above, though.

That header file can do the "no-inline" dance before including the
real thing with "#include_next", and nobody else would notice, no?

	#ifdef __NO_INLINE__
        #define __NO_INLINE_WAS_THERE 1
        #else
        #define __NO_INLINE__
        #define __NO_INLINE_WAS_THERE 0
	#endif

	#include_next <string.h>
        #if !__NO_INLINE_WAS_THERE
        #undef __NO_INLINE__
	#endif

or something like that.

That of course assumes nobody compiles for _MINGW32_ with a compiler
that does not understrand "#include_next" and I do not know if that
restriction is a showstopper or not.



>  #ifdef HAVE_STRINGS_H
>  #include <strings.h> /* for strcasecmp() */
>  #endif
> -#undef __NO_INLINE__
>
>  #ifdef WIN32 /* Both MinGW and MSVC */
>  #ifndef _WIN32_WINNT

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

[Sebastian Schuberth]

On Fri, Sep 13, 2013 at 4:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

>>> Perhaps the real issue is that the header file does not give an
>>> equivalent "those who want to take the address of strcasecmp will
>>> get the address of _stricmp instead" macro, e.g.
>>>
>>>         #define strcasecmp _stricmp
>>>
>>> or something?
>>
>> Now it's you who puzzles me, because the header file *does* have
>> exactly the macro that you suggest.
>
> Then why does your platform have problem with the code that takes
> the address of strcasecmp and stores it in the variable?  It is not
> me, but your platform that is puzzling us.
>
> There is something else going on, like you do not have that #define
> "enabled" under some condition, or something silly like that.

Exactly. That define is only enabled if __NO_INLINE__ is defined.
Which is what my patch is all about: Define __NO_INLINE__ so that we
get "#define strcasecmp _stricmp".

-- 
Sebastian Schuberth

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

[Sebastian Schuberth]

On Fri, Sep 13, 2013 at 4:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Which means people who do want to see that macro defined will be
> broken after that section of the header file which unconditionally
> undefs it, right?

Right, but luckily you've fixed that in our proposed patch :-)

> That is certainly better than the unconditional one, but I wonder if
> it is an option to add compat/mingw/string.h without doing the
> above, though.

I don't like the idea of introducing a compat/mingw/string.h because
of two reasons: You would have to add a conditional to include that
string.h instead of the system one anyway, so we could just as well
keep the conditional in git-compat-util.h along with the logic. And I
don't like the include_next GCC-ism, especially as I was planning to
take a look at compiling Git with LLVM / clang under Windows. So how
about this:

--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,6 +85,25 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1

+#ifdef __MINGW32__
+#ifdef __NO_INLINE__
+#define __NO_INLINE_ALREADY_DEFINED
+#else
+#define __NO_INLINE__ /* do not inline strcasecmp() */
+#endif
+#endif
+#include <string.h>
+#ifdef __MINGW32__
+#ifdef __NO_INLINE_ALREADY_DEFINED
+#undef __NO_INLINE_ALREADY_DEFINED
+#else
+#undef __NO_INLINE__
+#endif
+#endif
+#ifdef HAVE_STRINGS_H
+#include <strings.h> /* for strcasecmp() */
+#endif
+
 #ifdef WIN32 /* Both MinGW and MSVC */
 #define _WIN32_WINNT 0x0502
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
@@ -99,10 +118,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

-- 
Sebastian Schuberth

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

[Linus Torvalds]

On Fri, Sep 13, 2013 at 12:53 PM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
>
> +#ifdef __MINGW32__
> +#ifdef __NO_INLINE__

Why do you want to push this insane workaround for a clear Mingw bug?

Please have mingw just fix the nasty bug, and the git patch with the
trivial wrapper looks much simpler than just saying "don't inline
anything" and that crazy block of nasty mingw magic #defines/.

And then document loudly that the wrapper is due to the mingw bug.

               Linus

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> I don't like the idea of introducing a compat/mingw/string.h because
> of two reasons: You would have to add a conditional to include that
> string.h instead of the system one anyway,

With -Icompat/mingw passed to the compiler, which is a bog-standard
technique we already use to supply headers the system forgot to
supply or override buggy headers the system is shipped with, you do
not have to change any "#include <string.h>".

Am I mistaken?

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

[Sebastian Schuberth]

On Fri, Sep 13, 2013 at 9:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

>> +#ifdef __MINGW32__
>> +#ifdef __NO_INLINE__
>
> Why do you want to push this insane workaround for a clear Mingw bug?

To be frank, because Git is picking up patches much quicker than MinGW
does, and I want a solution ASAP. Although I of course agree that
fixing the real issue upstream in MinGW is the better solution.

> Please have mingw just fix the nasty bug, and the git patch with the

I'll try to come up with a MinGW patch in parallel.

> trivial wrapper looks much simpler than just saying "don't inline
> anything" and that crazy block of nasty mingw magic #defines/.

It may look simpler, but as outlines in this thread it's less
maintainable because you need to remember to use the wrapper. And
people tend to forget that no matter how loudly you document that. If
we can make the code more fool proof we IMHO should do so.

-- 
Sebastian Schuberth

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

[Sebastian Schuberth]

On Fri, Sep 13, 2013 at 10:01 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> I don't like the idea of introducing a compat/mingw/string.h because
>> of two reasons: You would have to add a conditional to include that
>> string.h instead of the system one anyway,
>
> With -Icompat/mingw passed to the compiler, which is a bog-standard
> technique we already use to supply headers the system forgot to
> supply or override buggy headers the system is shipped with, you do
> not have to change any "#include <string.h>".
>
> Am I mistaken?

Ah, that would work I guess, but you'd still need the include_next.

-- 
Sebastian Schuberth

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On Fri, Sep 13, 2013 at 10:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> I don't like the idea of introducing a compat/mingw/string.h because
>>> of two reasons: You would have to add a conditional to include that
>>> string.h instead of the system one anyway,
>>
>> With -Icompat/mingw passed to the compiler, which is a bog-standard
>> technique we already use to supply headers the system forgot to
>> supply or override buggy headers the system is shipped with, you do
>> not have to change any "#include <string.h>".
>>
>> Am I mistaken?
>
> Ah, that would work I guess, but you'd still need the include_next.

You can explicitly include the system header from your compatibility
layer, i.e. 

	=== compat/mingw/string.h ===

	#define __NO_INLINE__

	#ifdef SYSTEM_STRING_H_HEADER
        #include SYSTEM_STRING_H_HEADER
        #else
        #include_next <string.h>
	#endif

and then in config.mak.uname, do something like this:

	ifneq (,$(findstring MINGW,$(uname_S)))
	ifndef SYSTEM_STRING_H_HEADER
	SYSTEM_STRING_H_HEADER = "C:\\llvm\include\string.h"
        endif

	COMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER)
	endif

People who have the system header file at different paths can
further override SYSTEM_STRING_H_HEADER in their config.mak.

That would help compilers targetting mingw that do not support
"#include_next" without spreading the damage to other people's
systems, I think.

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

[Junio C Hamano]

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

> You can explicitly include the system header from your compatibility
> layer, i.e. 
> ...
> and then in config.mak.uname, do something like this:
> ...
> 	COMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER)

You need to have one level of quoting to keep "" from being eaten;
it should be sufficient to see how SHA1_HEADER that is included in
cache.h is handled and imitate it.

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

[Sebastian Schuberth]

On Sat, Sep 14, 2013 at 12:06 AM, Junio C Hamano <gitster@pobox.com> wrote:

> You can explicitly include the system header from your compatibility
> layer, i.e.
>
>         === compat/mingw/string.h ===
>
>         #define __NO_INLINE__
>
>         #ifdef SYSTEM_STRING_H_HEADER
>         #include SYSTEM_STRING_H_HEADER
>         #else
>         #include_next <string.h>
>         #endif
>
> and then in config.mak.uname, do something like this:
>
>         ifneq (,$(findstring MINGW,$(uname_S)))
>         ifndef SYSTEM_STRING_H_HEADER
>         SYSTEM_STRING_H_HEADER = "C:\\llvm\include\string.h"
>         endif
>
>         COMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER)
>         endif
>
> People who have the system header file at different paths can
> further override SYSTEM_STRING_H_HEADER in their config.mak.
>
> That would help compilers targetting mingw that do not support
> "#include_next" without spreading the damage to other people's
> systems, I think.

I think this is less favorable compared to my last proposed solution.
While my work-around in git-compat-util.h from [1] already is quite
ugly, it's at least in a single place. You solution spreads the code
it multiple place, making it even more ugly and less comprehensible,
IMHO.

[1] http://www.spinics.net/lists/git/msg217546.html

-- 
Sebastian Schuberth

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> I think this is less favorable compared to my last proposed solution.

That is only needed if you insist to use C preprocessor that does
not understand include_next.  That choice is a platform specific
decision (even if you want to use such a compiler on a platform it
may not have been ported to yours, etc.).

Keeping the ugliness to deal with the platform issue (i.e. broken
string.h) in one place (e.g. compat/mingw) is far more preferrable
than having a similar ugliness in git-compat-util.h for people on
all other platforms to see, no?

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

[Sebastian Schuberth]

On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Keeping the ugliness to deal with the platform issue (i.e. broken
> string.h) in one place (e.g. compat/mingw) is far more preferrable
> than having a similar ugliness in git-compat-util.h for people on
> all other platforms to see, no?

I don't think people on other platforms seeing the ugliness is really
an issue. After all, the file is called git-*compat*-util.h; I sort of
expect to see such things there, and I would expect only more complex
compatibility stuff that requires multiple files in the compat/
directory. Also, your solution does not really keep the ugliness in
one place, you need the change in config.mak.uname, too (because yes,
I do insist to avoid GCC-ism in C files, just like you probably would
insist to avoid Bash-ism in shell scripts).

-- 
Sebastian Schuberth

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

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Keeping the ugliness to deal with the platform issue (i.e. broken
>> string.h) in one place (e.g. compat/mingw) is far more preferrable
>> than having a similar ugliness in git-compat-util.h for people on
>> all other platforms to see, no?
>
> I don't think people on other platforms seeing the ugliness is really
> an issue. After all, the file is called git-*compat*-util.h;

Well, judging from the way Linus reacted to the patch, I'd have to
disagree.  After all, that argument leads to the position that
nothing is needed in compat/, no?

> Also, your solution does not really keep the ugliness in
> one place,...

One ugliness (lack of sane strcasecmp definition whose address can
be taken) specific to mingw is worked around in compat/mingw.h, and
another ugliness that some people may use compilers without include_next
may need help from another configuration in the Makefile to tell it
where the platform string.h resides.  I am not sure why you see it
as a problem.

> I do insist to avoid GCC-ism in C files,...

To that I tend to agree.  Unconditionally killing inlining for any
mingw compilation in compat/mingw.h may be the simplest (albeit it
may be less than optimal) solution.

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

[Sebastian Schuberth]

On Tue, Sep 17, 2013 at 11:46 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> I don't think people on other platforms seeing the ugliness is really
>> an issue. After all, the file is called git-*compat*-util.h;
>
> Well, judging from the way Linus reacted to the patch, I'd have to
> disagree.  After all, that argument leads to the position that
> nothing is needed in compat/, no?

My feeling is that Linus' reaction was more about that this
work-around is even necessary (and MinGW is buggy) rather than
applying it to git-compat-util.h and not elsewhere.

> One ugliness (lack of sane strcasecmp definition whose address can
> be taken) specific to mingw is worked around in compat/mingw.h, and
> another ugliness that some people may use compilers without include_next
> may need help from another configuration in the Makefile to tell it
> where the platform string.h resides.  I am not sure why you see it
> as a problem.

I just don't like that the ugliness is spreading out and requires a
change to config.mak.uname now, too. Also, I regard the change to
config.mak.uname by itself as ugly, mainly because you would have to
set SYSTEM_STRING_H_HEADER to some path, but that path might differ
from system to system, depending on where MinGW is installed on
Windows.

>> I do insist to avoid GCC-ism in C files,...
>
> To that I tend to agree.  Unconditionally killing inlining for any
> mingw compilation in compat/mingw.h may be the simplest (albeit it
> may be less than optimal) solution.

I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed,
it involved the need to shuffle includes in git-compat-util.h around
because winsock2.h already seems to include string.h, and I did not
find a working include order. So I came up with the following, do you
like that better?

diff --git a/compat/string_no_inline.h b/compat/string_no_inline.h
new file mode 100644
index 0000000..51eed52
--- /dev/null
+++ b/compat/string_no_inline.h
@@ -0,0 +1,25 @@
+#ifndef STRING_NO_INLINE_H
+#define STRING_NO_INLINE_H
+
+#ifdef __MINGW32__
+#ifdef __NO_INLINE__
+#define __NO_INLINE_ALREADY_DEFINED
+#else
+#define __NO_INLINE__ /* do not inline strcasecmp() */
+#endif
+#endif
+
+#include <string.h>
+#ifdef HAVE_STRINGS_H
+#include <strings.h> /* for strcasecmp() */
+#endif
+
+#ifdef __MINGW32__
+#ifdef __NO_INLINE_ALREADY_DEFINED
+#undef __NO_INLINE_ALREADY_DEFINED
+#else
+#undef __NO_INLINE__
+#endif
+#endif
+
+#endif
diff --git a/git-compat-util.h b/git-compat-util.h
index db564b7..348dd55 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,6 +85,8 @@
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1

+#include "compat/string_no_inline.h"
+
 #ifdef WIN32 /* Both MinGW and MSVC */
 #ifndef _WIN32_WINNT
 #define _WIN32_WINNT 0x0502
@@ -101,10 +103,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.dirty

-- 
Sebastian Schuberth

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

[Linus Torvalds]

On Wed, Sep 18, 2013 at 5:43 AM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
>
> My feeling is that Linus' reaction was more about that this
> work-around is even necessary (and MinGW is buggy) rather than
> applying it to git-compat-util.h and not elsewhere.

So I think it's an annoying MinGW bug, but the reason I dislike the
"no-inline" approach is two-fold:

 - it's *way* too intimate with the bug.

   When you have a bug like this, the *last* thing you want to do is
to make sweet sweet love to it, and get really involved with it.

   You want to say "Eww, what a nasty little bug, I don't want to have
anything to do with you".

   And quite frankly, delving into the details of exactly *what* MinGW
does wrong, and defining magic __NO_INLINE__ macros, knowing that that
is the particular incantation that hides the MinGW bug, that's being
too intimate. That's simply a level of detail that *nobody* should
ever have to know.

   The other patch (having just a wrapper function) doesn't have those
kinds of intimacy issues. That patch just says "MinGW is buggy and
cannot do this function uninlined, so we wrap it". Notice the lack of
detail, and lack of *interest* in the exact particular pattern of the
bug.

The other reason I'm not a fan of the __NO_INLINE__ approach is even
more straightforward:

 - Why should we disable the inlining of everything in <string.h> (and
possibly elsewhere too - who the hell knows what __NO_INLINE__ will do
to other header files), when in 99% of all the cases we don't care,
and in fact inlining may well be good and the right thing to do.

So the __NO_INLINE__ games seem to be both too big of a hammer, and
too non-specific, and at the same time it gets really intimate with
MinGW in unhealthy ways.

If you know something is diseased, you keep your distance, you don't
try to embrace it.

> I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed,
> it involved the need to shuffle includes in git-compat-util.h around
> because winsock2.h already seems to include string.h, and I did not
> find a working include order. So I came up with the following, do you
> like that better?

Ugh, so now that patch is fragile, so we have to complicate it even more.

Really, just make a wrapper function. It doesn't even need to be
conditional on MinGW. Just a single one-liner function, with a comment
above it that says "MinGW is broken and doesn't have an out-of-line
copy of strcasecmp(), so we wrap it here".

No unnecessary details about internal workings of a buggy MinGW header
file. No complexity. No subtle issues with include file ordering. Just
a straightforward workaround that is easy to explain.

                        Linus

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

[Piotr Krukowiecki]

On Wed, Sep 11, 2013 at 9:16 PM, Jeff King <peff@peff.net> 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

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

[Piotr Krukowiecki]

On Thu, Sep 19, 2013 at 9:37 AM, Junio C Hamano <junio@pobox.com> wrote:
> On Sep 18, 2013 11:08 PM, "Piotr Krukowiecki" <piotr.krukowiecki@gmail.com>
> wrote:
>>
>> On Wed, Sep 11, 2013 at 9:16 PM, Jeff King <peff@peff.net> wrote:
>> > I would prefer the static wrapper solution you suggest, though. It
[...]
>> > 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?
>
> No test needed for this, as compilation or linkage will fail, I think.

But only when someone compiles on MinGW, no?

-- 
Piotr Krukowiecki

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

[Sebastian Schuberth]

On 12.09.2013 23:31, Jonathan Nieder wrote:

>> And that's exactly what defining __NO_INLINE__ does. Granted, defining
>> __NO_INLINE__ in the scope of string.h will also add a "#define
>> strcasecmp _stricmp"; but despite it's name, defining __NO_INLINE__
>> does not imply a performance hit due to functions not being inlined
>> because it's just the "strncasecmp" wrapper around "_strnicmp" that's
>> being inlined, not "_strnicmp" itself.
>
> What I don't understand is why the header doesn't use "static inline"
> instead of "extern inline".  The former would seem to be better in
> every way for this particular use case.
>
> See also <http://www.greenend.org.uk/rjk/tech/inline.html>, section
> "GNU C inline rules".

I've suggested this at [1] now to see if such a patch is likely to be 
accepted.

[1] http://article.gmane.org/gmane.comp.gnu.mingw.user/42993

-- 
Sebastian Schuberth

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

[Jeff King]

On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote:

> >> > 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?
> >
> > No test needed for this, as compilation or linkage will fail, I think.
> 
> But only when someone compiles on MinGW, no?

Yeah. I think a more clear way to phrase the question would be: is there
some trick we can use to booby-trap strcasecmp as a function pointer so
that it fails to compile even on systems where it would otherwise work?

I can't think off-hand of a way to do so using preprocessor tricks, and
even if we could, I suspect the result would end up quite ugly. It's
probably enough to just catch such problems in review, or let people on
affected systems report and fix the error if it slips through.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote:
>
>> >> > 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?
>> >
>> > No test needed for this, as compilation or linkage will fail, I think.
>> 
>> But only when someone compiles on MinGW, no?
>
> Yeah. I think a more clear way to phrase the question would be: is there
> some trick we can use to booby-trap strcasecmp as a function pointer so
> that it fails to compile even on systems where it would otherwise work?

That line of thought nudges us toward the place Linus explicitly
said he didn't want to see us going, no?  We do not particularly
want to care the exact nature of the breakage on MinGW.  Do we
really want to set a booby-trap that intimately knows about how
their strcasecmp is broken, and possibly cover breakages of the same
kind but with other functions?

It isn't like "we are deliberately relying on this non-standard
behaviour we see on the system _we_ commonly use, and somebody on
a new strictly POSIX platform may be bitten by it", in which case it
would make sense to have a test that intimately knows about the
non-standard behaviour we rely on.  This case is a total opposite.

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

[Jeff King]

On Thu, Sep 19, 2013 at 03:03:46PM -0700, Junio C Hamano wrote:

> >> But only when someone compiles on MinGW, no?
> >
> > Yeah. I think a more clear way to phrase the question would be: is there
> > some trick we can use to booby-trap strcasecmp as a function pointer so
> > that it fails to compile even on systems where it would otherwise work?
> 
> That line of thought nudges us toward the place Linus explicitly
> said he didn't want to see us going, no?  We do not particularly
> want to care the exact nature of the breakage on MinGW.  Do we
> really want to set a booby-trap that intimately knows about how
> their strcasecmp is broken, and possibly cover breakages of the same
> kind but with other functions?

Exactly. You snipped my second paragraph, but the gist of it was "...and
no, we do not want to go there". Calling it a booby-trap was meant to be
derogatory. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... "...and
> no, we do not want to go there". Calling it a booby-trap was meant to be
> derogatory. :)

OK, I've resurrected the following and queued on 'pu'.

-- >8 --
Subject: [PATCH] mailmap: work around implementations with pure inline strcasecmp

On some systems (e.g. MinGW 4.0), string.h has only inline
definition of strcasecmp and no non-inline implementation is
supplied anywhere, which is, eh, "unusual".  We cannot take an
address of such a function to store it in namemap.cmp.

Work it around by introducing our own level of indirection.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mailmap.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 44614fc..91a7532 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -52,6 +52,20 @@ static void free_mailmap_entry(void *p, const char *s)
 	string_list_clear_func(&me->namemap, free_mailmap_info);
 }
 
+/*
+ * On some systems (e.g. MinGW 4.0), string.h has _only_ inline
+ * definition of strcasecmp and no non-inline implementation is
+ * supplied anywhere, which is, eh, "unusual"; we cannot take an
+ * address of such a function to store it in namemap.cmp.  This is
+ * here as a workaround---do not assign strcasecmp directly to
+ * namemap.cmp until we know no systems that matter have such an
+ * "unusual" string.h.
+ */
+static int namemap_cmp(const char *a, const char *b)
+{
+	return strcasecmp(a, b);
+}
+
 static void add_mapping(struct string_list *map,
 			char *new_name, char *new_email,
 			char *old_name, char *old_email)
@@ -75,7 +89,7 @@ static void add_mapping(struct string_list *map,
 		item = string_list_insert_at_index(map, index, old_email);
 		me = xcalloc(1, sizeof(struct mailmap_entry));
 		me->namemap.strdup_strings = 1;
-		me->namemap.cmp = strcasecmp;
+		me->namemap.cmp = namemap_cmp;
 		item->util = me;
 	}
 
@@ -241,7 +255,7 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)
 	int err = 0;
 
 	map->strdup_strings = 1;
-	map->cmp = strcasecmp;
+	map->cmp = namemap_cmp;
 
 	if (!git_mailmap_blob && is_bare_repository())
 		git_mailmap_blob = "HEAD:.mailmap";
-- 
1.8.4-613-ge7dc249

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

[Jeff King]

On Thu, Sep 19, 2013 at 03:40:05PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... "...and
> > no, we do not want to go there". Calling it a booby-trap was meant to be
> > derogatory. :)
> 
> OK, I've resurrected the following and queued on 'pu'.

Looks good to me.

-Peff

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

[Piotr Krukowiecki]

Jeff King <peff@peff.net> napisał:
>On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote:
>
>> >> > 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?
>> >
>> > No test needed for this, as compilation or linkage will fail, I
>think.
>> 
>> But only when someone compiles on MinGW, no?
>
>Yeah. I think a more clear way to phrase the question would be: is
>there
>some trick we can use to booby-trap strcasecmp as a function pointer so
>that it fails to compile even on systems where it would otherwise work?
>
>I can't think off-hand of a way to do so using preprocessor tricks, and
>even if we could, I suspect the result would end up quite ugly. 

What I meant was: can we add a test (in t/) which greps git source code and fails if it finds strcasecmp string? 

It could count number of strcasecmp and expect to find only 1 or exclude  known location of the wrapper. 


-- 
Piotr Krukowiecki 

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

[Jeff King]

On Fri, Sep 20, 2013 at 08:21:04AM +0200, Piotr Krukowiecki wrote:

> >I can't think off-hand of a way to do so using preprocessor tricks, and
> >even if we could, I suspect the result would end up quite ugly. 
> 
> What I meant was: can we add a test (in t/) which greps git source
> code and fails if it finds strcasecmp string?
> 
> It could count number of strcasecmp and expect to find only 1 or
> exclude  known location of the wrapper.

No, because it is perfectly fine (and desirable) to use strcasecmp as a
function, just not as a function pointer. Telling the difference would
involve minor parsing of C.

So I think the least bad thing is to simply catch it in review, or by
testing on affected platforms.

-Peff