Skip to contentSkip to conversation

git-compat-util: Avoid strcasecmp() being inlined

PATCH In review Sebastian Schuberth 7 participants 48 messages Jun 15, 2016 1 message not in the archive

lore
Share & tools
lore

Sebastian Schuberth

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
---
 git-compat-util.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
git-compat-util.h +7 -4
index be1c494..664305c 100644
@@ -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
Signature · 1 line
1.8.3.mingw.1.2.g56240b5.dirty
#1 lore raw
Quoted 2 lines · Sebastian Schuberth

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

#2 lore raw
Quoted 9 lines · Jonathan Nieder
Quoted 2 lines · Sebastian Schuberth

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

#3 lore raw
Quoted 7 lines · Jeff King

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?

Signature · 1 line
Piotr Krukowiecki

A message that is not in the archive (sent off-list or not yet received): CAPc5daVt4Q9twub5KyOQqZHx9CwOnkuwA97sXV44fF2j1e5HVg@mail.gmail.com

#5 lore raw
Quoted 5 lines · Junio C Hamano
Quoted 3 lines · Piotr Krukowiecki
Quoted 1 line · Jeff King

I would prefer the static wrapper solution you suggest, though. It

[...]

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

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?

Signature · 1 line
Piotr Krukowiecki
#6 lore raw
Quoted 9 lines · Piotr Krukowiecki
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

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

#7 lore raw
Quoted 15 lines · Jeff King
Quoted 1 line · Piotr Krukowiecki
Quoted 1 line
Quoted 1 line
Quoted 1 line

it still ends up as a single function call). The downside is

that it has

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

to be remembered at each site that uses strcasecmp, but we do

not use

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 2 lines

But only when someone compiles on MinGW, no?

Quoted 7 lines

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.

Signature · 1 line
Piotr Krukowiecki
#8 lore raw
Quoted 8 lines · Piotr Krukowiecki
Quoted 2 lines

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

#9 lore raw
In reply to Jeff King
Quoted 8 lines · Jeff King
Quoted 6 lines · Piotr Krukowiecki
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

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?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

#10 lore raw
Quoted 6 lines · Junio C Hamano
Quoted 5 lines
Quoted 1 line

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?

Quoted 6 lines

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

#11 lore raw
Quoted 3 lines · Jeff King

... "...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 <[email protected]>
---
 mailmap.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
mailmap.c +16 -2
index 44614fc..91a7532 100644
@@ -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";
Signature · 1 line
1.8.4-613-ge7dc249
#12 lore raw
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Jeff King

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

author
#13 lore raw
In reply to Jonathan Nieder
Quoted 7 lines · Jonathan Nieder
Quoted 2 lines

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

Signature · 1 line
Sebastian Schuberth
#14 lore raw
Quoted 13 lines · Sebastian Schuberth
Quoted 7 lines · Jonathan Nieder
Quoted 2 lines

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]):

Quoted 7 lines

#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

author
#15 lore raw
Quoted 11 lines · Jeff King
Quoted 10 lines

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

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 3 lines

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);}

Signature · 1 line
Sebastian Schuberth
#16 lore raw
Quoted 9 lines · Sebastian Schuberth
Quoted 3 lines

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

#17 lore raw
Quoted 7 lines · John Keeping
Quoted 6 lines · Sebastian Schuberth
Quoted 3 lines

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:

Quoted 8 lines
Quoted 3 lines

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(-)
mailmap.c +15 -2
index a7969c4..d36d424 100644
@@ -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";
#18 lore raw
Quoted 11 lines · Junio C Hamano
Quoted 4 lines

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

author
#19 lore raw
Quoted 12 lines · Jeff King
Quoted 11 lines
Quoted 4 lines

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.

Quoted 4 lines

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.

Signature · 1 line
Sebastian Schuberth
#20 lore raw
Quoted 5 lines · Sebastian Schuberth
Quoted 4 lines

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?

Quoted 12 lines

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

#21 lore raw
Quoted 13 lines · Jeff King

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

Quoted 8 lines

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.

author
#22 lore raw
Quoted 12 lines · Junio C Hamano
Quoted 10 lines · Jeff King

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.

Quoted 6 lines
Quoted 6 lines

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

Quoted 3 lines

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:

git-compat-util.h +5 -1
@@ -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
Signature · 1 line
Sebastian Schuberth
#23 lore raw
Quoted 5 lines · Sebastian Schuberth

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.

git-compat-util.h line 94

Quoted 13 lines
git-compat-util.h +5 -0
@@ -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.

Quoted 7 lines
git-compat-util.h +0 -1
 #ifdef HAVE_STRINGS_H
 #include <strings.h> /* for strcasecmp() */
 #endif
-#undef __NO_INLINE__
 
 #ifdef WIN32 /* Both MinGW and MSVC */
 #ifndef _WIN32_WINNT
author
#24 lore raw
Quoted 3 lines · Junio C Hamano

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

Quoted 3 lines

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:

git-compat-util.h +19 -4
@@ -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
Signature · 1 line
Sebastian Schuberth
#25 lore raw

git-compat-util.h line 89

Quoted 3 lines · Sebastian Schuberth
git-compat-util.h +2 -0
+#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

author
#26 lore raw
Quoted 4 lines · Linus Torvalds

git-compat-util.h line 89

Quoted 2 lines
git-compat-util.h +2 -0
+#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.

Quoted 1 line

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.

Quoted 2 lines

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.

Signature · 1 line
Sebastian Schuberth
#27 lore raw
In reply to Sebastian Schuberth
Quoted 3 lines · Sebastian Schuberth

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?

author
#28 lore raw
Quoted 10 lines · Junio C Hamano
Quoted 3 lines

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.

Signature · 1 line
Sebastian Schuberth
#29 lore raw
Quoted 6 lines · Sebastian Schuberth
Quoted 4 lines · Junio C Hamano
Quoted 3 lines

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,

Quoted 8 lines
Quoted 6 lines

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.

#30 lore raw
Quoted 6 lines · Junio C Hamano

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.

author
#31 lore raw
In reply to Junio C Hamano
Quoted 23 lines · Junio C Hamano

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
Quoted 6 lines

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

Signature · 1 line
Sebastian Schuberth
#32 lore raw
Quoted 1 line · Sebastian Schuberth

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?

author
#33 lore raw
Quoted 4 lines · Junio C Hamano

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

Signature · 1 line
Sebastian Schuberth
#34 lore raw
Quoted 9 lines · Sebastian Schuberth
Quoted 4 lines · Junio C Hamano

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?

Quoted 2 lines

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.

Quoted 1 line

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.

author
#35 lore raw
Quoted 6 lines · Junio C Hamano
Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 5 lines
Quoted 1 line

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?

compat/string_no_inline.h new file +25 -0
new file mode 100644
index 0000000..51eed52
@@ -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
git-compat-util.h +2 -4
index db564b7..348dd55 100644
@@ -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
Signature · 4 lines
1.8.3.mingw.1.dirty

-- 
Sebastian Schuberth
#36 lore raw
Quoted 4 lines · Sebastian Schuberth

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.

Quoted 5 lines

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

#37 lore raw
In reply to Sebastian Schuberth
Quoted 6 lines · Sebastian Schuberth

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

author
#38 lore raw
Quoted 7 lines · Jonathan Nieder
Quoted 6 lines

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.

Quoted 6 lines

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

Signature · 1 line
Sebastian Schuberth
#39 lore raw
In reply to Jeff King
Quoted 14 lines · Jeff King
Quoted 11 lines · Junio C Hamano
Quoted 4 lines

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.

Quoted 4 lines

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 <[email protected]>
---
 mailmap.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
mailmap.c +15 -2
index 44614fc..8863e23 100644
@@ -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";
Signature · 1 line
1.8.4-485-gec42fe2
Reviewed-by
#40 lore raw
Quoted 1 line · Junio C Hamano

I think we would want something like below.

Looks good to me, but

Quoted 4 lines

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

[...]

mailmap.c static void free_mailmap_entry(void *p, const char *s) · line 56

Quoted 8 lines
mailmap.c +2 -0
@@ -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 <[email protected]>
author
#41 lore raw
Quoted 8 lines · Jonathan Nieder

Looks good to me, but

Quoted 4 lines

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

Signature · 1 line
Sebastian Schuberth
#42 lore raw
Quoted 5 lines · Sebastian Schuberth

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?

author
#43 lore raw
Quoted 8 lines · Junio C Hamano
Quoted 5 lines

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

Quoted 1 line

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.

Quoted 7 lines

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.

Signature · 1 line
Sebastian Schuberth
#44 lore raw
Quoted 8 lines · Sebastian Schuberth
Quoted 6 lines · Junio C Hamano
Quoted 5 lines

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.

Quoted 4 lines
Quoted 2 lines

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.

Quoted 10 lines
Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 6 lines
Quoted 3 lines
        #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.

author
#45 lore raw
Quoted 11 lines · Junio C Hamano
Quoted 10 lines
Quoted 7 lines

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.

Quoted 6 lines

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

Signature · 1 line
Sebastian Schuberth
#46 lore raw
In reply to Sebastian Schuberth
Quoted 4 lines · Sebastian Schuberth

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

#47 lore raw
In reply to Junio C Hamano
Quoted 5 lines · Junio C Hamano
Quoted 5 lines
Quoted 4 lines

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

Quoted 7 lines
Quoted 4 lines

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

#48 lore raw
In reply to Sebastian Schuberth
Quoted 2 lines · Sebastian Schuberth

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.

Quoted 22 lines
git-compat-util.h +7 -4
+#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