{"thread":{"id":"34909","subject":"[PATCH] git-compat-util: Avoid strcasecmp() being inlined","startedAt":"2013-09-11T16:06:08Z","lastAt":"2013-09-24T05:32:30Z","messageCount":48,"participants":["Sebastian Schuberth","Jonathan Nieder","Junio C Hamano","Jeff King","John Keeping","Linus Torvalds","Piotr Krukowiecki"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"227459","messageId":"523094F0.9000509@gmail.com","threadId":"34909","inReplyTo":null,"subject":"[PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-11T16:06:08Z","receivedAt":"2013-09-11T16:06:08Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"This is necessary so that read_mailmap() can obtain a pointer to the\nfunction.\n\nSigned-off-by: Sebastian Schuberth \n---\n git-compat-util.h | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex be1c494..664305c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -85,6 +85,13 @@\n #define _NETBSD_SOURCE 1\n #define _SGI_SOURCE 1\n \n+#define __NO_INLINE__ /* do not inline strcasecmp() */\n+#include \n+#ifdef HAVE_STRINGS_H\n+#include /* for strcasecmp() */\n+#endif\n+#undef __NO_INLINE__\n+\n #ifdef WIN32 /* Both MinGW and MSVC */\n #define _WIN32_WINNT 0x0502\n #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n@@ -99,10 +106,6 @@\n #include \n #include \n #include \n-#include \n-#ifdef HAVE_STRINGS_H\n-#include /* for strcasecmp() */\n-#endif\n #include \n #include \n #ifdef NEEDS_SYS_PARAM_H\n-- \n1.8.3.mingw.1.2.g56240b5.dirty\n"},{"id":"227478","messageId":"20130911182921.GE4326@google.com","threadId":"34909","inReplyTo":"523094F0.9000509@gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-11T18:29:21Z","receivedAt":"2013-09-11T18:29:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sebastian Schuberth wrote:\n\n> This is necessary so that read_mailmap() can obtain a pointer to the\n> function.\n\nHm, what platform has strcasecmp() as an inline function? Is this\nallowed by POSIX? Even if it isn't, should we perhaps just work\naround it by providing our own thin static function wrapper in\nmailmap.c?\n\nCurious,\nJonathan\n"},{"id":"227480","messageId":"xmqqzjrjryms.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"523094F0.9000509@gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-11T18:39:39Z","receivedAt":"2013-09-11T18:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> This is necessary so that read_mailmap() can obtain a pointer to the\n> function.\n\nWhoa, I didn't think it is even legal for a C library to supply\nstrcmp() or strcasecmp() that are purely inline you cannot take the\naddress of. The \"solution\" looks a bit too large a hammer that\naffects everybody, not just those who have such a set of header\nfiles.\n\n> \n> +#define __NO_INLINE__ /* do not inline strcasecmp() */\n> +#include \n> +#ifdef HAVE_STRINGS_H\n> +#include /* for strcasecmp() */\n> +#endif\n> +#undef __NO_INLINE__\n> +\n> #ifdef WIN32 /* Both MinGW and MSVC */\n> #define _WIN32_WINNT 0x0502\n> #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n> @@ -99,10 +106,6 @@\n> #include \n> #include \n> #include \n> -#include \n> -#ifdef HAVE_STRINGS_H\n> -#include /* for strcasecmp() */\n> -#endif\n> #include \n> #include \n> #ifdef NEEDS_SYS_PARAM_H\n"},{"id":"227485","messageId":"20130911191620.GB24251@sigill.intra.peff.net","threadId":"34909","inReplyTo":"20130911182921.GE4326@google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-11T19:16:20Z","receivedAt":"2013-09-11T19:16:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2013 at 11:29:21AM -0700, Jonathan Nieder wrote:\n\n> Sebastian Schuberth wrote:\n> \n> > This is necessary so that read_mailmap() can obtain a pointer to the\n> > function.\n> \n> Hm, what platform has strcasecmp() as an inline function? Is this\n> allowed by POSIX? Even if it isn't, should we perhaps just work\n> around it by providing our own thin static function wrapper in\n> mailmap.c?\n\nEnvironments can implement library functions as macros or even\nintrinsics, but C99 requires that they still allow you to access a\nfunction pointer. And if my reading of C99 6.7.4 is correct, it should\napply to inlines, too, because you should always be able to take the\naddress of an inline function (though it is a little subtle).\n\nBut that does not mean there are not popular platforms that we do not\nhave to workaround (and the inline keyword is C99 anyway, so all bets\nare off for pre-C99 inline implementations).\n\nI would prefer the static wrapper solution you suggest, though. It\nleaves the compiler free to optimize the common case of normal\nstrcasecmp calls, and only introduces an extra function indirection when\nusing it as a callback (and even then, if we can inline the strcasecmp,\nit still ends up as a single function call). The downside is that it has\nto be remembered at each site that uses strcasecmp, but we do not use\npointers to standard library functions very often.\n\n-Peff\n"},{"id":"227489","messageId":"CAHGBnuN0pSmX7_mM6xpRqpF4qPVbP7oBK416NrTVM7tu=DZTjg@mail.gmail.com","threadId":"34909","inReplyTo":"20130911182921.GE4326@google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-11T19:59:53Z","receivedAt":"2013-09-11T19:59:53Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder wrote:\n\n>> This is necessary so that read_mailmap() can obtain a pointer to the\n>> function.\n>\n> Hm, what platform has strcasecmp() as an inline function? Is this\n> allowed by POSIX? Even if it isn't, should we perhaps just work\n> around it by providing our own thin static function wrapper in\n> mailmap.c?\n\nI'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,\nstring.h contains the following code (see [1]):\n\n#ifndef __NO_INLINE__\n__CRT_INLINE int __cdecl __MINGW_NOTHROW\nstrncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)\n{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}\n#else\n#define strncasecmp _strnicmp\n#endif\n\n[1] http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/include/string.h#l107\n\n-- \nSebastian Schuberth\n"},{"id":"227494","messageId":"20130911214116.GA12235@sigill.intra.peff.net","threadId":"34909","inReplyTo":"CAHGBnuN0pSmX7_mM6xpRqpF4qPVbP7oBK416NrTVM7tu=DZTjg@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-11T21:41:16Z","receivedAt":"2013-09-11T21:41:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 11, 2013 at 09:59:53PM +0200, Sebastian Schuberth wrote:\n\n> On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder wrote:\n> \n> >> This is necessary so that read_mailmap() can obtain a pointer to the\n> >> function.\n> >\n> > Hm, what platform has strcasecmp() as an inline function? Is this\n> > allowed by POSIX? Even if it isn't, should we perhaps just work\n> > around it by providing our own thin static function wrapper in\n> > mailmap.c?\n> \n> I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,\n> string.h contains the following code (see [1]):\n> \n> #ifndef __NO_INLINE__\n> __CRT_INLINE int __cdecl __MINGW_NOTHROW\n> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)\n> {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}\n> #else\n> #define strncasecmp _strnicmp\n> #endif\n\nWhat is the error the compiler reports? Can it take the address of other\ninline functions? For example, can it compile:\n\n inline int foo(void) { return 5; }\n extern int bar(int (*cb)(void));\n int call(void) { return bar(foo); }\n\nJust wondering if that is the root of the problem, or if maybe there is\nsomething else subtle going on. Also, does __CRT_INLINE just turn into\n\"inline\", or is there perhaps some other pre-processor magic going on?\n\n-Peff\n"},{"id":"227516","messageId":"CAHGBnuP3iX9pqm5kK9_WjAXr5moDuJ1jxtUkXwKEt2jjLTcLkQ@mail.gmail.com","threadId":"34909","inReplyTo":"20130911214116.GA12235@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-12T09:36:56Z","receivedAt":"2013-09-12T09:36:56Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Sep 11, 2013 at 11:41 PM, Jeff King wrote:\n\n>> I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,\n>> string.h contains the following code (see [1]):\n>>\n>> #ifndef __NO_INLINE__\n>> __CRT_INLINE int __cdecl __MINGW_NOTHROW\n>> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)\n>> {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}\n>> #else\n>> #define strncasecmp _strnicmp\n>> #endif\n>\n> What is the error the compiler reports? Can it take the address of other\n\nThe error message of GCC 4.8.1 is:\n\n LINK git-credential-store.exe\nlibgit.a(mailmap.o): In function `read_mailmap':\nC:\\mingwGitDevEnv\\git/mailmap.c:238: undefined reference to `strcasecmp'\ncollect2.exe: error: ld returned 1 exit status\nmake: *** [git-credential-store.exe] Error 1\n\nSo it's a linker error, not a compiler error.\n\n> inline functions? For example, can it compile:\n>\n> inline int foo(void) { return 5; }\n> extern int bar(int (*cb)(void));\n> int call(void) { return bar(foo); }\n\nI had to modify the example slightly to:\n\ninline int foo(void) { return 5; }\nextern int bar(int (*cb)(void)) { return cb(); }\nint main(void) { return bar(foo); }\n\nAnd this compiles.\n\n> Just wondering if that is the root of the problem, or if maybe there is\n> something else subtle going on. Also, does __CRT_INLINE just turn into\n> \"inline\", or is there perhaps some other pre-processor magic going on?\n\nThis is the function definition from string.h after preprocessing:\n\nextern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))\nstrncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)\n {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}\n\n-- \nSebastian Schuberth\n"},{"id":"227521","messageId":"20130912101419.GY2582@serenity.lan","threadId":"34909","inReplyTo":"CAHGBnuP3iX9pqm5kK9_WjAXr5moDuJ1jxtUkXwKEt2jjLTcLkQ@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T10:14:19Z","receivedAt":"2013-09-12T10:14:19Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:\n> > Just wondering if that is the root of the problem, or if maybe there is\n> > something else subtle going on. Also, does __CRT_INLINE just turn into\n> > \"inline\", or is there perhaps some other pre-processor magic going on?\n> \n> This is the function definition from string.h after preprocessing:\n> \n> extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))\n> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)\n> {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}\n\nI wonder if GCC has changed it's behaviour to more closely match C99.\nClang as a compatibility article about this sort of issue:\n\n http://clang.llvm.org/compatibility.html#inline\n"},{"id":"227537","messageId":"xmqq61u6qcez.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"20130912101419.GY2582@serenity.lan","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T15:37:08Z","receivedAt":"2013-09-12T15:37:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:\n>> > Just wondering if that is the root of the problem, or if maybe there is\n>> > something else subtle going on. Also, does __CRT_INLINE just turn into\n>> > \"inline\", or is there perhaps some other pre-processor magic going on?\n>> \n>> This is the function definition from string.h after preprocessing:\n>> \n>> extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))\n>> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)\n>> {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}\n>\n> I wonder if GCC has changed it's behaviour to more closely match C99.\n> Clang as a compatibility article about this sort of issue:\n>\n> http://clang.llvm.org/compatibility.html#inline\n\nInteresting. The ways the page suggests as fixes are\n\n - change it to a \"statis inline\";\n - remove \"inline\" from the definition;\n - provide an external (non-inline) def somewhere else;\n - compile with gnu899 dialect.\n\nBut the first two are non-starter, and the third one to force\neverybody to define an equivalent implementation is nonsense, for a\ndefinition in the standard header file.\n\nI agree with an earlier conclusion that defining our own wrapper\n(with an explanation why such a redundant wrapper exists) is the\nbest course of action at this point, until the system header is\nfixed.\n\n mailmap.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex a7969c4..d36d424 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)\n \tstring_list_clear_func(&me->namemap, free_mailmap_info);\n }\n \n+/*\n+ * On some systems, string.h has _only_ inline definition of strcasecmp\n+ * without supplying a non-inline implementation anywhere, which is, eh,\n+ * \"unusual\"; we cannot take an address of such a function to store it in\n+ * namemap.cmp. This is here as a workaround---do not assign strcasecmp\n+ * directly to namemap.cmp until we know no systems that matter have such\n+ * an \"unusual\" string.h.\n+ */\n+static int namemap_cmp(const char *a, const char *b)\n+{\n+\treturn strcasecmp(a, b);\n+}\n+\n static void add_mapping(struct string_list *map,\n \t\t\tchar *new_name, char *new_email,\n \t\t\tchar *old_name, char *old_email)\n@@ -75,7 +88,7 @@ static void add_mapping(struct string_list *map,\n \t\titem = string_list_insert_at_index(map, index, old_email);\n \t\tme = xcalloc(1, sizeof(struct mailmap_entry));\n \t\tme->namemap.strdup_strings = 1;\n-\t\tme->namemap.cmp = strcasecmp;\n+\t\tme->namemap.cmp = namemap_cmp;\n \t\titem->util = me;\n \t}\n \n@@ -237,7 +250,7 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)\n \tint err = 0;\n \n \tmap->strdup_strings = 1;\n-\tmap->cmp = strcasecmp;\n+\tmap->cmp = namemap_cmp;\n \n \tif (!git_mailmap_blob && is_bare_repository())\n \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n"},{"id":"227547","messageId":"20130912182057.GB32069@sigill.intra.peff.net","threadId":"34909","inReplyTo":"xmqq61u6qcez.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-12T18:20:57Z","receivedAt":"2013-09-12T18:20:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2013 at 08:37:08AM -0700, Junio C Hamano wrote:\n\n> > I wonder if GCC has changed it's behaviour to more closely match C99.\n> > Clang as a compatibility article about this sort of issue:\n> >\n> > http://clang.llvm.org/compatibility.html#inline\n> \n> Interesting. The ways the page suggests as fixes are\n> \n> - change it to a \"statis inline\";\n> - remove \"inline\" from the definition;\n> - provide an external (non-inline) def somewhere else;\n> - compile with gnu899 dialect.\n\nRight, option 3 seems perfectly reasonable to me, as we must be prepared\nto cope with a decision not to inline the function, and there has to be\n_some_ linked implementation. But shouldn't libc be providing an\nexternal, linkable strcasecmp in this case?\n\n-Peff\n"},{"id":"227550","messageId":"xmqqd2odq45y.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"20130912182057.GB32069@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T18:35:21Z","receivedAt":"2013-09-12T18:35:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 12, 2013 at 08:37:08AM -0700, Junio C Hamano wrote:\n>\n>> > I wonder if GCC has changed it's behaviour to more closely match C99.\n>> > Clang as a compatibility article about this sort of issue:\n>> >\n>> > http://clang.llvm.org/compatibility.html#inline\n>> \n>> Interesting. The ways the page suggests as fixes are\n>> \n>> - change it to a \"statis inline\";\n>> - remove \"inline\" from the definition;\n>> - provide an external (non-inline) def somewhere else;\n>> - compile with gnu899 dialect.\n>\n> Right, option 3 seems perfectly reasonable to me, as we must be prepared\n> to cope with a decision not to inline the function, and there has to be\n> _some_ linked implementation. But shouldn't libc be providing an\n> external, linkable strcasecmp in this case?\n\nThat is exactly my point when I said that the third one is nonsense\nfor a definition in the standard header file.\n\nI think we would want something like below.\n\n-- >8 --\nSubject: [PATCH] mailmap: work around implementations with pure inline strcasecmp\n\nOn some systems, string.h has _only_ inline definition of strcasecmp\nwithout supplying a non-inline implementation anywhere, which is,\neh, \"unusual\"; we cannot take an address of such a function to store\nit in namemap.cmp. Work it around by introducing our own level of\nindirection.\n\nSigned-off-by: Junio C Hamano \n---\n mailmap.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 44614fc..8863e23 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)\n \tstring_list_clear_func(&me->namemap, free_mailmap_info);\n }\n \n+/*\n+ * On some systems, string.h has _only_ inline definition of strcasecmp\n+ * without supplying a non-inline implementation anywhere, which is, eh,\n+ * \"unusual\"; we cannot take an address of such a function to store it in\n+ * namemap.cmp. This is here as a workaround---do not assign strcasecmp\n+ * directly to namemap.cmp until we know no systems that matter have such\n+ * an \"unusual\" string.h.\n+ */\n+static int namemap_cmp(const char *a, const char *b)\n+{\n+\treturn strcasecmp(a, b);\n+}\n+\n static void add_mapping(struct string_list *map,\n \t\t\tchar *new_name, char *new_email,\n \t\t\tchar *old_name, char *old_email)\n@@ -75,7 +88,7 @@ static void add_mapping(struct string_list *map,\n \t\titem = string_list_insert_at_index(map, index, old_email);\n \t\tme = xcalloc(1, sizeof(struct mailmap_entry));\n \t\tme->namemap.strdup_strings = 1;\n-\t\tme->namemap.cmp = strcasecmp;\n+\t\tme->namemap.cmp = namemap_cmp;\n \t\titem->util = me;\n \t}\n \n@@ -241,7 +254,7 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)\n \tint err = 0;\n \n \tmap->strdup_strings = 1;\n-\tmap->cmp = strcasecmp;\n+\tmap->cmp = namemap_cmp;\n \n \tif (!git_mailmap_blob && is_bare_repository())\n \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n-- \n1.8.4-485-gec42fe2\n"},{"id":"227551","messageId":"20130912183849.GI4326@google.com","threadId":"34909","inReplyTo":"xmqqd2odq45y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-12T18:38:49Z","receivedAt":"2013-09-12T18:38:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> I think we would want something like below.\n\nLooks good to me, but\n\n> -- >8 --\n> Subject: [PATCH] mailmap: work around implementations with pure inline strcasecmp\n>\n> On some systems, string.h has _only_ inline definition of strcasecmp\n\nPlease specify which system we are talking about: s/some systems/MinGW 4.0/\n\n[...]\n> --- a/mailmap.c\n> +++ b/mailmap.c\n> @@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)\n> \tstring_list_clear_func(&me->namemap, free_mailmap_info);\n> }\n> \n> +/*\n> + * On some systems, string.h has _only_ inline definition of strcasecmp\n\nLikewise.\n\nWith or without that change,\nReviewed-by: Jonathan Nieder \n"},{"id":"227554","messageId":"20130912190019.GB636@sigill.intra.peff.net","threadId":"34909","inReplyTo":"xmqqd2odq45y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-12T19:00:19Z","receivedAt":"2013-09-12T19:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2013 at 11:35:21AM -0700, Junio C Hamano wrote:\n\n> >> - change it to a \"statis inline\";\n> >> - remove \"inline\" from the definition;\n> >> - provide an external (non-inline) def somewhere else;\n> >> - compile with gnu899 dialect.\n> >\n> > Right, option 3 seems perfectly reasonable to me, as we must be prepared\n> > to cope with a decision not to inline the function, and there has to be\n> > _some_ linked implementation. But shouldn't libc be providing an\n> > external, linkable strcasecmp in this case?\n> \n> That is exactly my point when I said that the third one is nonsense\n> for a definition in the standard header file.\n\nYes, but I am saying it is the responsibility of libc. IOW, I am\nwondering if this particular mingw environment is simply broken, and if\nso, what is the status on the fix? Could another option be to declare\nthe environment unworkable and tell people to upgrade?\n\nI am not even sure if we are right to call it broken, but talking to the\nmingw people might be a good next step, as they will surely have an\nopinion. :)\n\n-Peff\n"},{"id":"227563","messageId":"CAHGBnuPzzokV7YMrx0gAL1VACcmaLwFoaB3n6bX8Y-UDHs7S8A@mail.gmail.com","threadId":"34909","inReplyTo":"20130912182057.GB32069@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-12T19:46:51Z","receivedAt":"2013-09-12T19:46:51Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Sep 12, 2013 at 8:20 PM, Jeff King wrote:\n\n>> > I wonder if GCC has changed it's behaviour to more closely match C99.\n>> > Clang as a compatibility article about this sort of issue:\n>> >\n>> > http://clang.llvm.org/compatibility.html#inline\n>>\n>> Interesting. The ways the page suggests as fixes are\n>>\n>> - change it to a \"statis inline\";\n>> - remove \"inline\" from the definition;\n>> - provide an external (non-inline) def somewhere else;\n>> - compile with gnu899 dialect.\n>\n> Right, option 3 seems perfectly reasonable to me, as we must be prepared\n> to cope with a decision not to inline the function, and there has to be\n> _some_ linked implementation. But shouldn't libc be providing an\n> external, linkable strcasecmp in this case?\n\nMinGW / GCC is not linking against libc, but against MSVCRT, Visual\nStudio's C runtime. And in fact MSVCRT has a non-inline implementation\nof a \"case-insensitive string comparison for up to the first n\ncharacters\"; it just happens to be called \"_strnicmp\", not\n\"strncasecmp\". Which is why I still think just having a \"#define\nstrncasecmp _strnicmp\" is the most elegant solution to the problem.\nAnd that's exactly what defining __NO_INLINE__ does. Granted, defining\n__NO_INLINE__ in the scope of string.h will also add a \"#define\nstrcasecmp _stricmp\"; but despite it's name, defining __NO_INLINE__\ndoes not imply a performance hit due to functions not being inlined\nbecause it's just the \"strncasecmp\" wrapper around \"_strnicmp\" that's\nbeing inlined, not \"_strnicmp\" itself.\n\n-- \nSebastian Schuberth\n"},{"id":"227565","messageId":"CAHGBnuPejvs_zTdV52GWVCF35+Bdih2c1zNuBdHJRd_2ShcnKQ@mail.gmail.com","threadId":"34909","inReplyTo":"20130912183849.GI4326@google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-12T19:51:31Z","receivedAt":"2013-09-12T19:51:31Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Sep 12, 2013 at 8:38 PM, Jonathan Nieder wrote:\n\n> Looks good to me, but\n>\n>> -- >8 --\n>> Subject: [PATCH] mailmap: work around implementations with pure inline strcasecmp\n>>\n>> On some systems, string.h has _only_ inline definition of strcasecmp\n>\n> Please specify which system we are talking about: s/some systems/MinGW 4.0/\n\nI'm not too happy with the wording either. As I see it, even on MinGW\nruntime version 4.0 it's not true that \"string.h has _only_ inline\ndefinition of strcasecmp\"; there's also \"#define strncasecmp\n_strnicmp\" which effectively provides a non-inline definition of\nstrncasecmp aka _strnicmp.\n\n-- \nSebastian Schuberth\n"},{"id":"227569","messageId":"xmqqvc25ol9n.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuPejvs_zTdV52GWVCF35+Bdih2c1zNuBdHJRd_2ShcnKQ@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T20:08:52Z","receivedAt":"2013-09-12T20:08:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> I'm not too happy with the wording either. As I see it, even on MinGW\n> runtime version 4.0 it's not true that \"string.h has _only_ inline\n> definition of strcasecmp\"; there's also \"#define strncasecmp\n> _strnicmp\" which effectively provides a non-inline definition of\n> strncasecmp aka _strnicmp.\n\nI do not get this part. Sure, string.h would have definitions of\nthings other than strcasecmp, such as strncasecmp. So what?\n\nDoes it \"effectively\" provide a non-inline definition of strcasecmp?\n\nPerhaps the real issue is that the header file does not give an\nequivalent \"those who want to take the address of strcasecmp will\nget the address of _stricmp instead\" macro, e.g.\n\n\t#define strcasecmp _stricmp\n\nor something?\n"},{"id":"227572","messageId":"20130912202246.GF32069@sigill.intra.peff.net","threadId":"34909","inReplyTo":"CAHGBnuPzzokV7YMrx0gAL1VACcmaLwFoaB3n6bX8Y-UDHs7S8A@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-12T20:22:46Z","receivedAt":"2013-09-12T20:22:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2013 at 09:46:51PM +0200, Sebastian Schuberth wrote:\n\n> > Right, option 3 seems perfectly reasonable to me, as we must be prepared\n> > to cope with a decision not to inline the function, and there has to be\n> > _some_ linked implementation. But shouldn't libc be providing an\n> > external, linkable strcasecmp in this case?\n> \n> MinGW / GCC is not linking against libc, but against MSVCRT, Visual\n> Studio's C runtime. And in fact MSVCRT has a non-inline implementation\n> of a \"case-insensitive string comparison for up to the first n\n> characters\"; it just happens to be called \"_strnicmp\", not\n> \"strncasecmp\". Which is why I still think just having a \"#define\n> strncasecmp _strnicmp\" is the most elegant solution to the problem.\n> And that's exactly what defining __NO_INLINE__ does. Granted, defining\n> __NO_INLINE__ in the scope of string.h will also add a \"#define\n> strcasecmp _stricmp\"; but despite it's name, defining __NO_INLINE__\n> does not imply a performance hit due to functions not being inlined\n> because it's just the \"strncasecmp\" wrapper around \"_strnicmp\" that's\n> being inlined, not \"_strnicmp\" itself.\n\nAh, thanks, that explains what is going on. I do think the environment\nis probably in violation of C99, but I dug in the mingw history, and it\nlooks like it has been this way for over 10 years.\n\nSo it is probably worth working around, but it would be nice if the\ndamage could be contained to just the affected platform.\n\nI think there are basically three classes of solution:\n\n 1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other\n environments, who would then not inline and lose performance (but\n since it's a non-standard macro, we don't really know what it will\n do in other places; possibly nothing).\n\n 2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it\n only affects mingw, and we know the meaning of NO_INLINE there.\n\n 3. Try to impact only the uses as a function pointer (e.g., by using\n a wrapper function as suggested in the thread).\n\nYour patch does (1), I believe. Junio's patch does (3), but is a\nmaintenance burden in that any new callsites will need to remember to do\nthe same trick.\n\nBut your argument (and reading the mingw header, I agree) is that there\nis no performance difference at all between (2) and (3). And (2) does\nnot have the maintenance burden. So it does seem like the right path to\nme.\n\n-Peff\n"},{"id":"227573","messageId":"xmqqr4ctokat.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"20130912202246.GF32069@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T20:29:46Z","receivedAt":"2013-09-12T20:29:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think there are basically three classes of solution:\n>\n> 1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other\n> environments, who would then not inline and lose performance (but\n> since it's a non-standard macro, we don't really know what it will\n> do in other places; possibly nothing).\n>\n> 2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it\n> only affects mingw, and we know the meaning of NO_INLINE there.\n>\n> 3. Try to impact only the uses as a function pointer (e.g., by using\n> a wrapper function as suggested in the thread).\n>\n> Your patch does (1), I believe. Junio's patch does (3), but is a\n> maintenance burden in that any new callsites will need to remember to do\n> the same trick.\n>\n> But your argument (and reading the mingw header, I agree) is that there\n> is no performance difference at all between (2) and (3). And (2) does\n> not have the maintenance burden. So it does seem like the right path to\n> me.\n\nAgreed. If that #define __NO_INLINE__ does not appear in the common\npart of our header files like git-compat-util.h but is limited to\nsomewhere in compat/, that would be the perfect outcome.\n\nThanks, both.\n"},{"id":"227581","messageId":"20130912213149.GK4326@google.com","threadId":"34909","inReplyTo":"CAHGBnuPzzokV7YMrx0gAL1VACcmaLwFoaB3n6bX8Y-UDHs7S8A@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-12T21:31:49Z","receivedAt":"2013-09-12T21:31:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sebastian Schuberth wrote:\n\n> And that's exactly what defining __NO_INLINE__ does. Granted, defining\n> __NO_INLINE__ in the scope of string.h will also add a \"#define\n> strcasecmp _stricmp\"; but despite it's name, defining __NO_INLINE__\n> does not imply a performance hit due to functions not being inlined\n> because it's just the \"strncasecmp\" wrapper around \"_strnicmp\" that's\n> being inlined, not \"_strnicmp\" itself.\n\nWhat I don't understand is why the header doesn't use \"static inline\"\ninstead of \"extern inline\". The former would seem to be better in\nevery way for this particular use case.\n\nSee also , section\n\"GNU C inline rules\".\n\nThanks,\nJonathan\n"},{"id":"227582","messageId":"20130912213633.GL4326@google.com","threadId":"34909","inReplyTo":"CAHGBnuPejvs_zTdV52GWVCF35+Bdih2c1zNuBdHJRd_2ShcnKQ@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-12T21:36:33Z","receivedAt":"2013-09-12T21:36:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sebastian Schuberth wrote:\n\n> I'm not too happy with the wording either. As I see it, even on MinGW\n> runtime version 4.0 it's not true that \"string.h has _only_ inline\n> definition of strcasecmp\"; there's also \"#define strncasecmp\n> _strnicmp\"\n\nI assume you mean \"#define strcasecmp _stricmp\", which is guarded by\ndefined(__NO_INLINE__). I think what Junio meant is that by default\n(i.e., in the !defined(__NO_INLINE__) case) string.h uses\n__CRT_INLINE, defined as\n\n\textern inline __attribute__((__gnu_inline__))\n\nto suppress the non-inline function definition.\n\nJonathan\n"},{"id":"227630","messageId":"CAHGBnuN+HkZt48Pg2sHnYAhYW7EufWhO6rfgKpgaSOGeGA0Z4w@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqvc25ol9n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-13T12:33:01Z","receivedAt":"2013-09-13T12:33:01Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Sep 12, 2013 at 10:08 PM, Junio C Hamano wrote:\n\n>> I'm not too happy with the wording either. As I see it, even on MinGW\n>> runtime version 4.0 it's not true that \"string.h has _only_ inline\n>> definition of strcasecmp\"; there's also \"#define strncasecmp\n>> _strnicmp\" which effectively provides a non-inline definition of\n>> strncasecmp aka _strnicmp.\n>\n> I do not get this part. Sure, string.h would have definitions of\n> things other than strcasecmp, such as strncasecmp. So what?\n\nSorry, I mixed up \"strcasecmp\" and \"strncasecmp\".\n\n> Does it \"effectively\" provide a non-inline definition of strcasecmp?\n\nYes, if __NO_INLINE__ is defined string.h provides non-inline\ndefinition of both \"strcasecmp\" and \"strncasecmp\" by defining them to\n\"_stricmp\" and \"_strnicmp\" respectively.\n\n> Perhaps the real issue is that the header file does not give an\n> equivalent \"those who want to take the address of strcasecmp will\n> get the address of _stricmp instead\" macro, e.g.\n>\n> #define strcasecmp _stricmp\n>\n> or something?\n\nNow it's you who puzzles me, because the header file *does* have\nexactly the macro that you suggest.\n\nAnyway, I think Peff's reply to my other mail summed it up nicely. I\nwill come up with another patch.\n\n-- \nSebastian Schuberth\n"},{"id":"227631","messageId":"CAHGBnuOQ-y1beD_X_jiH+FrhPvLOVJqT0J=Wk988Q4NeCs1-9Q@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqr4ctokat.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-13T12:47:52Z","receivedAt":"2013-09-13T12:47:52Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Sep 12, 2013 at 10:29 PM, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> I think there are basically three classes of solution:\n>>\n>> 1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other\n>> environments, who would then not inline and lose performance (but\n>> since it's a non-standard macro, we don't really know what it will\n>> do in other places; possibly nothing).\n>>\n>> 2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it\n>> only affects mingw, and we know the meaning of NO_INLINE there.\n>>\n>> 3. Try to impact only the uses as a function pointer (e.g., by using\n>> a wrapper function as suggested in the thread).\n>>\n>> Your patch does (1), I believe. Junio's patch does (3), but is a\n>> maintenance burden in that any new callsites will need to remember to do\n>> the same trick.\n\nWell, if by \"everywhere\" in (1) you mean \"on all platforms\" then\nyou're right. But my patch does not define __NO_INLINE__ globally, but\nonly at the time string.h / strings.h is included. Afterwards\n__NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not\ndefined \"everywhere\".\n\n> Agreed. If that #define __NO_INLINE__ does not appear in the common\n> part of our header files like git-compat-util.h but is limited to\n> somewhere in compat/, that would be the perfect outcome.\n\nIt's not that easy to move the definition of __NO_INLINE__ into\ncompat/ because git-compat-util.h includes string.h / strings.h before\nanything of compat/. More over, defining __NO_INLINE__ in somewhere in\ncompat/ would not limit its definition to the string.h / strings.h\nheaders only. So how about something like this on top of my original\npatch:\n\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -85,12 +85,16 @@\n #define _NETBSD_SOURCE 1\n #define _SGI_SOURCE 1\n\n+#ifdef __MINGW32__\n #define __NO_INLINE__ /* do not inline strcasecmp() */\n+#endif\n #include \n+#ifdef __MINGW32__\n+#undef __NO_INLINE__\n+#endif\n #ifdef HAVE_STRINGS_H\n #include /* for strcasecmp() */\n #endif\n-#undef __NO_INLINE__\n\n #ifdef WIN32 /* Both MinGW and MSVC */\n #ifndef _WIN32_WINNT\n\n-- \nSebastian Schuberth\n"},{"id":"227635","messageId":"xmqqtxhokdao.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuN+HkZt48Pg2sHnYAhYW7EufWhO6rfgKpgaSOGeGA0Z4w@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T14:26:55Z","receivedAt":"2013-09-13T14:26:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Thu, Sep 12, 2013 at 10:08 PM, Junio C Hamano wrote:\n>\n>>> I'm not too happy with the wording either. As I see it, even on MinGW\n>>> runtime version 4.0 it's not true that \"string.h has _only_ inline\n>>> definition of strcasecmp\"; there's also \"#define strncasecmp\n>>> _strnicmp\" which effectively provides a non-inline definition of\n>>> strncasecmp aka _strnicmp.\n>>\n>> I do not get this part. Sure, string.h would have definitions of\n>> things other than strcasecmp, such as strncasecmp. So what?\n>\n> Sorry, I mixed up \"strcasecmp\" and \"strncasecmp\".\n\nOK.\n\n>> Does it \"effectively\" provide a non-inline definition of strcasecmp?\n>\n> Yes, if __NO_INLINE__ is defined string.h provides non-inline\n> definition of both \"strcasecmp\" and \"strncasecmp\" by defining them to\n> \"_stricmp\" and \"_strnicmp\" respectively.\n>\n>> Perhaps the real issue is that the header file does not give an\n>> equivalent \"those who want to take the address of strcasecmp will\n>> get the address of _stricmp instead\" macro, e.g.\n>>\n>> #define strcasecmp _stricmp\n>>\n>> or something?\n>\n> Now it's you who puzzles me, because the header file *does* have\n> exactly the macro that you suggest.\n\nThen why does your platform have problem with the code that takes\nthe address of strcasecmp and stores it in the variable? It is not\nme, but your platform that is puzzling us.\n\nThere is something else going on, like you do not have that #define\n\"enabled\" under some condition, or something silly like that.\n"},{"id":"227636","messageId":"xmqqppsckcsd.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuOQ-y1beD_X_jiH+FrhPvLOVJqT0J=Wk988Q4NeCs1-9Q@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T14:37:54Z","receivedAt":"2013-09-13T14:37:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> Well, if by \"everywhere\" in (1) you mean \"on all platforms\" then\n> you're right. But my patch does not define __NO_INLINE__ globally, but\n> only at the time string.h / strings.h is included. Afterwards\n> __NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not\n> defined \"everywhere\".\n\nWhich means people who do want to see that macro defined will be\nbroken after that section of the header file which unconditionally\nundefs it, right?\n\nThat is exactly why that change should not appear in the platform\nneutral part of the header file.\n\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -85,12 +85,16 @@\n> #define _NETBSD_SOURCE 1\n> #define _SGI_SOURCE 1\n>\n> +#ifdef __MINGW32__\n> #define __NO_INLINE__ /* do not inline strcasecmp() */\n> +#endif\n> #include \n> +#ifdef __MINGW32__\n> +#undef __NO_INLINE__\n> +#endif\n\nThat is certainly better than the unconditional one, but I wonder if\nit is an option to add compat/mingw/string.h without doing the\nabove, though.\n\nThat header file can do the \"no-inline\" dance before including the\nreal thing with \"#include_next\", and nobody else would notice, no?\n\n\t#ifdef __NO_INLINE__\n #define __NO_INLINE_WAS_THERE 1\n #else\n #define __NO_INLINE__\n #define __NO_INLINE_WAS_THERE 0\n\t#endif\n\n\t#include_next \n #if !__NO_INLINE_WAS_THERE\n #undef __NO_INLINE__\n\t#endif\n\nor something like that.\n\nThat of course assumes nobody compiles for _MINGW32_ with a compiler\nthat does not understrand \"#include_next\" and I do not know if that\nrestriction is a showstopper or not.\n\n\n\n> #ifdef HAVE_STRINGS_H\n> #include /* for strcasecmp() */\n> #endif\n> -#undef __NO_INLINE__\n>\n> #ifdef WIN32 /* Both MinGW and MSVC */\n> #ifndef _WIN32_WINNT\n"},{"id":"227643","messageId":"CAHGBnuNu7G+6T8jySwNfkjrUaQZzwxf5ginq+x0S1erY6L_ZoQ@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqtxhokdao.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-13T19:34:05Z","receivedAt":"2013-09-13T19:34:05Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Sep 13, 2013 at 4:26 PM, Junio C Hamano wrote:\n\n>>> Perhaps the real issue is that the header file does not give an\n>>> equivalent \"those who want to take the address of strcasecmp will\n>>> get the address of _stricmp instead\" macro, e.g.\n>>>\n>>> #define strcasecmp _stricmp\n>>>\n>>> or something?\n>>\n>> Now it's you who puzzles me, because the header file *does* have\n>> exactly the macro that you suggest.\n>\n> Then why does your platform have problem with the code that takes\n> the address of strcasecmp and stores it in the variable? It is not\n> me, but your platform that is puzzling us.\n>\n> There is something else going on, like you do not have that #define\n> \"enabled\" under some condition, or something silly like that.\n\nExactly. That define is only enabled if __NO_INLINE__ is defined.\nWhich is what my patch is all about: Define __NO_INLINE__ so that we\nget \"#define strcasecmp _stricmp\".\n\n-- \nSebastian Schuberth\n"},{"id":"227645","messageId":"CAHGBnuMNDJhAqNfgVRHRE-7R=UZbd+fMExYeKDWWCFjyQJYYTQ@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqppsckcsd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-13T19:53:04Z","receivedAt":"2013-09-13T19:53:04Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Sep 13, 2013 at 4:37 PM, Junio C Hamano wrote:\n\n> Which means people who do want to see that macro defined will be\n> broken after that section of the header file which unconditionally\n> undefs it, right?\n\nRight, but luckily you've fixed that in our proposed patch :-)\n\n> That is certainly better than the unconditional one, but I wonder if\n> it is an option to add compat/mingw/string.h without doing the\n> above, though.\n\nI don't like the idea of introducing a compat/mingw/string.h because\nof two reasons: You would have to add a conditional to include that\nstring.h instead of the system one anyway, so we could just as well\nkeep the conditional in git-compat-util.h along with the logic. And I\ndon't like the include_next GCC-ism, especially as I was planning to\ntake a look at compiling Git with LLVM / clang under Windows. So how\nabout this:\n\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -85,6 +85,25 @@\n #define _NETBSD_SOURCE 1\n #define _SGI_SOURCE 1\n\n+#ifdef __MINGW32__\n+#ifdef __NO_INLINE__\n+#define __NO_INLINE_ALREADY_DEFINED\n+#else\n+#define __NO_INLINE__ /* do not inline strcasecmp() */\n+#endif\n+#endif\n+#include \n+#ifdef __MINGW32__\n+#ifdef __NO_INLINE_ALREADY_DEFINED\n+#undef __NO_INLINE_ALREADY_DEFINED\n+#else\n+#undef __NO_INLINE__\n+#endif\n+#endif\n+#ifdef HAVE_STRINGS_H\n+#include /* for strcasecmp() */\n+#endif\n+\n #ifdef WIN32 /* Both MinGW and MSVC */\n #define _WIN32_WINNT 0x0502\n #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n@@ -99,10 +118,6 @@\n #include \n #include \n #include \n-#include \n-#ifdef HAVE_STRINGS_H\n-#include /* for strcasecmp() */\n-#endif\n #include \n #include \n #ifdef NEEDS_SYS_PARAM_H\n\n-- \nSebastian Schuberth\n"},{"id":"227646","messageId":"CA+55aFws7iNGRpu6wKBkKj_5bw3vu_E+q+1__Aw4kmhkaUMRGw@mail.gmail.com","threadId":"34909","inReplyTo":"CAHGBnuMNDJhAqNfgVRHRE-7R=UZbd+fMExYeKDWWCFjyQJYYTQ@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2013-09-13T19:56:50Z","receivedAt":"2013-09-13T19:56:50Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Sep 13, 2013 at 12:53 PM, Sebastian Schuberth\n wrote:\n>\n> +#ifdef __MINGW32__\n> +#ifdef __NO_INLINE__\n\nWhy do you want to push this insane workaround for a clear Mingw bug?\n\nPlease have mingw just fix the nasty bug, and the git patch with the\ntrivial wrapper looks much simpler than just saying \"don't inline\nanything\" and that crazy block of nasty mingw magic #defines/.\n\nAnd then document loudly that the wrapper is due to the mingw bug.\n\n Linus\n"},{"id":"227647","messageId":"xmqqppscij8a.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuMNDJhAqNfgVRHRE-7R=UZbd+fMExYeKDWWCFjyQJYYTQ@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T20:01:41Z","receivedAt":"2013-09-13T20:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> I don't like the idea of introducing a compat/mingw/string.h because\n> of two reasons: You would have to add a conditional to include that\n> string.h instead of the system one anyway,\n\nWith -Icompat/mingw passed to the compiler, which is a bog-standard\ntechnique we already use to supply headers the system forgot to\nsupply or override buggy headers the system is shipped with, you do\nnot have to change any \"#include \".\n\nAm I mistaken?\n"},{"id":"227648","messageId":"CAHGBnuNQFRHunX9wkBpS1GHXVRP+LL2YOr38fyX_J=5TWP5jgw@mail.gmail.com","threadId":"34909","inReplyTo":"CA+55aFws7iNGRpu6wKBkKj_5bw3vu_E+q+1__Aw4kmhkaUMRGw@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-13T20:03:23Z","receivedAt":"2013-09-13T20:03:23Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Sep 13, 2013 at 9:56 PM, Linus Torvalds\n wrote:\n\n>> +#ifdef __MINGW32__\n>> +#ifdef __NO_INLINE__\n>\n> Why do you want to push this insane workaround for a clear Mingw bug?\n\nTo be frank, because Git is picking up patches much quicker than MinGW\ndoes, and I want a solution ASAP. Although I of course agree that\nfixing the real issue upstream in MinGW is the better solution.\n\n> Please have mingw just fix the nasty bug, and the git patch with the\n\nI'll try to come up with a MinGW patch in parallel.\n\n> trivial wrapper looks much simpler than just saying \"don't inline\n> anything\" and that crazy block of nasty mingw magic #defines/.\n\nIt may look simpler, but as outlines in this thread it's less\nmaintainable because you need to remember to use the wrapper. And\npeople tend to forget that no matter how loudly you document that. If\nwe can make the code more fool proof we IMHO should do so.\n\n-- \nSebastian Schuberth\n"},{"id":"227649","messageId":"CAHGBnuM=QqLxPNNZmoL1jG+oAm2y6o=AuBtkH+FRwZ_8ahGC+w@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqppscij8a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-13T20:04:47Z","receivedAt":"2013-09-13T20:04:47Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Sep 13, 2013 at 10:01 PM, Junio C Hamano wrote:\n\n>> I don't like the idea of introducing a compat/mingw/string.h because\n>> of two reasons: You would have to add a conditional to include that\n>> string.h instead of the system one anyway,\n>\n> With -Icompat/mingw passed to the compiler, which is a bog-standard\n> technique we already use to supply headers the system forgot to\n> supply or override buggy headers the system is shipped with, you do\n> not have to change any \"#include \".\n>\n> Am I mistaken?\n\nAh, that would work I guess, but you'd still need the include_next.\n\n-- \nSebastian Schuberth\n"},{"id":"227656","messageId":"xmqqli30idfx.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuM=QqLxPNNZmoL1jG+oAm2y6o=AuBtkH+FRwZ_8ahGC+w@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T22:06:42Z","receivedAt":"2013-09-13T22:06:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Fri, Sep 13, 2013 at 10:01 PM, Junio C Hamano wrote:\n>\n>>> I don't like the idea of introducing a compat/mingw/string.h because\n>>> of two reasons: You would have to add a conditional to include that\n>>> string.h instead of the system one anyway,\n>>\n>> With -Icompat/mingw passed to the compiler, which is a bog-standard\n>> technique we already use to supply headers the system forgot to\n>> supply or override buggy headers the system is shipped with, you do\n>> not have to change any \"#include \".\n>>\n>> Am I mistaken?\n>\n> Ah, that would work I guess, but you'd still need the include_next.\n\nYou can explicitly include the system header from your compatibility\nlayer, i.e. \n\n\t=== compat/mingw/string.h ===\n\n\t#define __NO_INLINE__\n\n\t#ifdef SYSTEM_STRING_H_HEADER\n #include SYSTEM_STRING_H_HEADER\n #else\n #include_next \n\t#endif\n\nand then in config.mak.uname, do something like this:\n\n\tifneq (,$(findstring MINGW,$(uname_S)))\n\tifndef SYSTEM_STRING_H_HEADER\n\tSYSTEM_STRING_H_HEADER = \"C:\\\\llvm\\include\\string.h\"\n endif\n\n\tCOMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER)\n\tendif\n\nPeople who have the system header file at different paths can\nfurther override SYSTEM_STRING_H_HEADER in their config.mak.\n\nThat would help compilers targetting mingw that do not support\n\"#include_next\" without spreading the damage to other people's\nsystems, I think.\n"},{"id":"227664","messageId":"xmqqtxhogxk2.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"xmqqli30idfx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-13T22:35:09Z","receivedAt":"2013-09-13T22:35:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> You can explicitly include the system header from your compatibility\n> layer, i.e. \n> ...\n> and then in config.mak.uname, do something like this:\n> ...\n> \tCOMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER)\n\nYou need to have one level of quoting to keep \"\" from being eaten;\nit should be sufficient to see how SHA1_HEADER that is included in\ncache.h is handled and imitate it.\n"},{"id":"227693","messageId":"CAHGBnuOfYoosgWQdfF+L3=YCqO-MYEx-TpNzBAD-Zt0kqeR_Hw@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqli30idfx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-15T12:44:03Z","receivedAt":"2013-09-15T12:44:03Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Sat, Sep 14, 2013 at 12:06 AM, Junio C Hamano wrote:\n\n> You can explicitly include the system header from your compatibility\n> layer, i.e.\n>\n> === compat/mingw/string.h ===\n>\n> #define __NO_INLINE__\n>\n> #ifdef SYSTEM_STRING_H_HEADER\n> #include SYSTEM_STRING_H_HEADER\n> #else\n> #include_next \n> #endif\n>\n> and then in config.mak.uname, do something like this:\n>\n> ifneq (,$(findstring MINGW,$(uname_S)))\n> ifndef SYSTEM_STRING_H_HEADER\n> SYSTEM_STRING_H_HEADER = \"C:\\\\llvm\\include\\string.h\"\n> endif\n>\n> COMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER)\n> endif\n>\n> People who have the system header file at different paths can\n> further override SYSTEM_STRING_H_HEADER in their config.mak.\n>\n> That would help compilers targetting mingw that do not support\n> \"#include_next\" without spreading the damage to other people's\n> systems, I think.\n\nI think this is less favorable compared to my last proposed solution.\nWhile my work-around in git-compat-util.h from [1] already is quite\nugly, it's at least in a single place. You solution spreads the code\nit multiple place, making it even more ugly and less comprehensible,\nIMHO.\n\n[1] http://www.spinics.net/lists/git/msg217546.html\n\n-- \nSebastian Schuberth\n"},{"id":"227753","messageId":"xmqqhadj1kyo.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuOfYoosgWQdfF+L3=YCqO-MYEx-TpNzBAD-Zt0kqeR_Hw@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T16:17:35Z","receivedAt":"2013-09-17T16:17:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> I think this is less favorable compared to my last proposed solution.\n\nThat is only needed if you insist to use C preprocessor that does\nnot understand include_next. That choice is a platform specific\ndecision (even if you want to use such a compiler on a platform it\nmay not have been ported to yours, etc.).\n\nKeeping the ugliness to deal with the platform issue (i.e. broken\nstring.h) in one place (e.g. compat/mingw) is far more preferrable\nthan having a similar ugliness in git-compat-util.h for people on\nall other platforms to see, no?\n"},{"id":"227775","messageId":"CAHGBnuMgE1zO4=MnJJXcDLJSD2Vsjptk1x2Bc6CpF9GSxmFp8w@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqhadj1kyo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-17T19:16:13Z","receivedAt":"2013-09-17T19:16:13Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano wrote:\n\n> Keeping the ugliness to deal with the platform issue (i.e. broken\n> string.h) in one place (e.g. compat/mingw) is far more preferrable\n> than having a similar ugliness in git-compat-util.h for people on\n> all other platforms to see, no?\n\nI don't think people on other platforms seeing the ugliness is really\nan issue. After all, the file is called git-*compat*-util.h; I sort of\nexpect to see such things there, and I would expect only more complex\ncompatibility stuff that requires multiple files in the compat/\ndirectory. Also, your solution does not really keep the ugliness in\none place, you need the change in config.mak.uname, too (because yes,\nI do insist to avoid GCC-ism in C files, just like you probably would\ninsist to avoid Bash-ism in shell scripts).\n\n-- \nSebastian Schuberth\n"},{"id":"227804","messageId":"xmqqsix3w27t.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"CAHGBnuMgE1zO4=MnJJXcDLJSD2Vsjptk1x2Bc6CpF9GSxmFp8w@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T21:46:46Z","receivedAt":"2013-09-17T21:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano wrote:\n>\n>> Keeping the ugliness to deal with the platform issue (i.e. broken\n>> string.h) in one place (e.g. compat/mingw) is far more preferrable\n>> than having a similar ugliness in git-compat-util.h for people on\n>> all other platforms to see, no?\n>\n> I don't think people on other platforms seeing the ugliness is really\n> an issue. After all, the file is called git-*compat*-util.h;\n\nWell, judging from the way Linus reacted to the patch, I'd have to\ndisagree. After all, that argument leads to the position that\nnothing is needed in compat/, no?\n\n> Also, your solution does not really keep the ugliness in\n> one place,...\n\nOne ugliness (lack of sane strcasecmp definition whose address can\nbe taken) specific to mingw is worked around in compat/mingw.h, and\nanother ugliness that some people may use compilers without include_next\nmay need help from another configuration in the Makefile to tell it\nwhere the platform string.h resides. I am not sure why you see it\nas a problem.\n\n> I do insist to avoid GCC-ism in C files,...\n\nTo that I tend to agree. Unconditionally killing inlining for any\nmingw compilation in compat/mingw.h may be the simplest (albeit it\nmay be less than optimal) solution.\n"},{"id":"227830","messageId":"CAHGBnuMh9wqe6mhLyqbPAGJUEEH7cA2LZPuCQK8VD=NU2ix3Pg@mail.gmail.com","threadId":"34909","inReplyTo":"xmqqsix3w27t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-18T09:43:11Z","receivedAt":"2013-09-18T09:43:11Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Sep 17, 2013 at 11:46 PM, Junio C Hamano wrote:\n\n>> I don't think people on other platforms seeing the ugliness is really\n>> an issue. After all, the file is called git-*compat*-util.h;\n>\n> Well, judging from the way Linus reacted to the patch, I'd have to\n> disagree. After all, that argument leads to the position that\n> nothing is needed in compat/, no?\n\nMy feeling is that Linus' reaction was more about that this\nwork-around is even necessary (and MinGW is buggy) rather than\napplying it to git-compat-util.h and not elsewhere.\n\n> One ugliness (lack of sane strcasecmp definition whose address can\n> be taken) specific to mingw is worked around in compat/mingw.h, and\n> another ugliness that some people may use compilers without include_next\n> may need help from another configuration in the Makefile to tell it\n> where the platform string.h resides. I am not sure why you see it\n> as a problem.\n\nI just don't like that the ugliness is spreading out and requires a\nchange to config.mak.uname now, too. Also, I regard the change to\nconfig.mak.uname by itself as ugly, mainly because you would have to\nset SYSTEM_STRING_H_HEADER to some path, but that path might differ\nfrom system to system, depending on where MinGW is installed on\nWindows.\n\n>> I do insist to avoid GCC-ism in C files,...\n>\n> To that I tend to agree. Unconditionally killing inlining for any\n> mingw compilation in compat/mingw.h may be the simplest (albeit it\n> may be less than optimal) solution.\n\nI tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed,\nit involved the need to shuffle includes in git-compat-util.h around\nbecause winsock2.h already seems to include string.h, and I did not\nfind a working include order. So I came up with the following, do you\nlike that better?\n\ndiff --git a/compat/string_no_inline.h b/compat/string_no_inline.h\nnew file mode 100644\nindex 0000000..51eed52\n--- /dev/null\n+++ b/compat/string_no_inline.h\n@@ -0,0 +1,25 @@\n+#ifndef STRING_NO_INLINE_H\n+#define STRING_NO_INLINE_H\n+\n+#ifdef __MINGW32__\n+#ifdef __NO_INLINE__\n+#define __NO_INLINE_ALREADY_DEFINED\n+#else\n+#define __NO_INLINE__ /* do not inline strcasecmp() */\n+#endif\n+#endif\n+\n+#include \n+#ifdef HAVE_STRINGS_H\n+#include /* for strcasecmp() */\n+#endif\n+\n+#ifdef __MINGW32__\n+#ifdef __NO_INLINE_ALREADY_DEFINED\n+#undef __NO_INLINE_ALREADY_DEFINED\n+#else\n+#undef __NO_INLINE__\n+#endif\n+#endif\n+\n+#endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex db564b7..348dd55 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -85,6 +85,8 @@\n #define _NETBSD_SOURCE 1\n #define _SGI_SOURCE 1\n\n+#include \"compat/string_no_inline.h\"\n+\n #ifdef WIN32 /* Both MinGW and MSVC */\n #ifndef _WIN32_WINNT\n #define _WIN32_WINNT 0x0502\n@@ -101,10 +103,6 @@\n #include \n #include \n #include \n-#include \n-#ifdef HAVE_STRINGS_H\n-#include /* for strcasecmp() */\n-#endif\n #include \n #include \n #ifdef NEEDS_SYS_PARAM_H\n-- \n1.8.3.mingw.1.dirty\n\n-- \nSebastian Schuberth\n"},{"id":"227833","messageId":"CA+55aFwJQ7yo3N3rdAz2=o9Zxxt4ascF5kzvB6-YHL+HXbz7ug@mail.gmail.com","threadId":"34909","inReplyTo":"CAHGBnuMh9wqe6mhLyqbPAGJUEEH7cA2LZPuCQK8VD=NU2ix3Pg@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2013-09-18T12:19:08Z","receivedAt":"2013-09-18T12:19:08Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Sep 18, 2013 at 5:43 AM, Sebastian Schuberth\n wrote:\n>\n> My feeling is that Linus' reaction was more about that this\n> work-around is even necessary (and MinGW is buggy) rather than\n> applying it to git-compat-util.h and not elsewhere.\n\nSo I think it's an annoying MinGW bug, but the reason I dislike the\n\"no-inline\" approach is two-fold:\n\n - it's *way* too intimate with the bug.\n\n When you have a bug like this, the *last* thing you want to do is\nto make sweet sweet love to it, and get really involved with it.\n\n You want to say \"Eww, what a nasty little bug, I don't want to have\nanything to do with you\".\n\n And quite frankly, delving into the details of exactly *what* MinGW\ndoes wrong, and defining magic __NO_INLINE__ macros, knowing that that\nis the particular incantation that hides the MinGW bug, that's being\ntoo intimate. That's simply a level of detail that *nobody* should\never have to know.\n\n The other patch (having just a wrapper function) doesn't have those\nkinds of intimacy issues. That patch just says \"MinGW is buggy and\ncannot do this function uninlined, so we wrap it\". Notice the lack of\ndetail, and lack of *interest* in the exact particular pattern of the\nbug.\n\nThe other reason I'm not a fan of the __NO_INLINE__ approach is even\nmore straightforward:\n\n - Why should we disable the inlining of everything in (and\npossibly elsewhere too - who the hell knows what __NO_INLINE__ will do\nto other header files), when in 99% of all the cases we don't care,\nand in fact inlining may well be good and the right thing to do.\n\nSo the __NO_INLINE__ games seem to be both too big of a hammer, and\ntoo non-specific, and at the same time it gets really intimate with\nMinGW in unhealthy ways.\n\nIf you know something is diseased, you keep your distance, you don't\ntry to embrace it.\n\n> I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed,\n> it involved the need to shuffle includes in git-compat-util.h around\n> because winsock2.h already seems to include string.h, and I did not\n> find a working include order. So I came up with the following, do you\n> like that better?\n\nUgh, so now that patch is fragile, so we have to complicate it even more.\n\nReally, just make a wrapper function. It doesn't even need to be\nconditional on MinGW. Just a single one-liner function, with a comment\nabove it that says \"MinGW is broken and doesn't have an out-of-line\ncopy of strcasecmp(), so we wrap it here\".\n\nNo unnecessary details about internal workings of a buggy MinGW header\nfile. No complexity. No subtle issues with include file ordering. Just\na straightforward workaround that is easy to explain.\n\n Linus\n"},{"id":"227871","messageId":"CAA01CsrN+VLw4WQmObvh72_MoH1Lyh9dQbizJcVhqyJoRyms-Q@mail.gmail.com","threadId":"34909","inReplyTo":"20130911191620.GB24251@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2013-09-19T06:04:40Z","receivedAt":"2013-09-19T06:04:40Z","isPatch":true,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"On Wed, Sep 11, 2013 at 9:16 PM, Jeff King wrote:\n> I would prefer the static wrapper solution you suggest, though. It\n> leaves the compiler free to optimize the common case of normal\n> strcasecmp calls, and only introduces an extra function indirection when\n> using it as a callback (and even then, if we can inline the strcasecmp,\n> it still ends up as a single function call). The downside is that it has\n> to be remembered at each site that uses strcasecmp, but we do not use\n> pointers to standard library functions very often.\n\nIs it possible to add a test which fails if wrapper is not used?\n\n-- \nPiotr Krukowiecki\n"},{"id":"227879","messageId":"CAA01CspCWFMGxXs9M3A1mtTctiUCCeJ9pJjHt=auMjhHHJU3Dg@mail.gmail.com","threadId":"34909","inReplyTo":"CAPc5daVt4Q9twub5KyOQqZHx9CwOnkuwA97sXV44fF2j1e5HVg@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2013-09-19T09:47:51Z","receivedAt":"2013-09-19T09:47:51Z","isPatch":true,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"On Thu, Sep 19, 2013 at 9:37 AM, Junio C Hamano wrote:\n> On Sep 18, 2013 11:08 PM, \"Piotr Krukowiecki\" \n> wrote:\n>>\n>> On Wed, Sep 11, 2013 at 9:16 PM, Jeff King wrote:\n>> > I would prefer the static wrapper solution you suggest, though. It\n[...]\n>> > it still ends up as a single function call). The downside is that it has\n>> > to be remembered at each site that uses strcasecmp, but we do not use\n>> > pointers to standard library functions very often.\n>>\n>> Is it possible to add a test which fails if wrapper is not used?\n>\n> No test needed for this, as compilation or linkage will fail, I think.\n\nBut only when someone compiles on MinGW, no?\n\n-- \nPiotr Krukowiecki\n"},{"id":"227886","messageId":"523B0079.6000404@gmail.com","threadId":"34909","inReplyTo":"20130912213149.GK4326@google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-09-19T13:47:37Z","receivedAt":"2013-09-19T13:47:37Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 12.09.2013 23:31, Jonathan Nieder wrote:\n\n>> And that's exactly what defining __NO_INLINE__ does. Granted, defining\n>> __NO_INLINE__ in the scope of string.h will also add a \"#define\n>> strcasecmp _stricmp\"; but despite it's name, defining __NO_INLINE__\n>> does not imply a performance hit due to functions not being inlined\n>> because it's just the \"strncasecmp\" wrapper around \"_strnicmp\" that's\n>> being inlined, not \"_strnicmp\" itself.\n>\n> What I don't understand is why the header doesn't use \"static inline\"\n> instead of \"extern inline\". The former would seem to be better in\n> every way for this particular use case.\n>\n> See also , section\n> \"GNU C inline rules\".\n\nI've suggested this at [1] now to see if such a patch is likely to be \naccepted.\n\n[1] http://article.gmane.org/gmane.comp.gnu.mingw.user/42993\n\n-- \nSebastian Schuberth\n"},{"id":"227900","messageId":"20130919211659.GB16556@sigill.intra.peff.net","threadId":"34909","inReplyTo":"CAA01CspCWFMGxXs9M3A1mtTctiUCCeJ9pJjHt=auMjhHHJU3Dg@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-19T21:16:59Z","receivedAt":"2013-09-19T21:16:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote:\n\n> >> > it still ends up as a single function call). The downside is that it has\n> >> > to be remembered at each site that uses strcasecmp, but we do not use\n> >> > pointers to standard library functions very often.\n> >>\n> >> Is it possible to add a test which fails if wrapper is not used?\n> >\n> > No test needed for this, as compilation or linkage will fail, I think.\n> \n> But only when someone compiles on MinGW, no?\n\nYeah. I think a more clear way to phrase the question would be: is there\nsome trick we can use to booby-trap strcasecmp as a function pointer so\nthat it fails to compile even on systems where it would otherwise work?\n\nI can't think off-hand of a way to do so using preprocessor tricks, and\neven if we could, I suspect the result would end up quite ugly. It's\nprobably enough to just catch such problems in review, or let people on\naffected systems report and fix the error if it slips through.\n\n-Peff\n"},{"id":"227902","messageId":"xmqqy56sqxj1.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"20130919211659.GB16556@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-19T22:03:46Z","receivedAt":"2013-09-19T22:03:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote:\n>\n>> >> > it still ends up as a single function call). The downside is that it has\n>> >> > to be remembered at each site that uses strcasecmp, but we do not use\n>> >> > pointers to standard library functions very often.\n>> >>\n>> >> Is it possible to add a test which fails if wrapper is not used?\n>> >\n>> > No test needed for this, as compilation or linkage will fail, I think.\n>> \n>> But only when someone compiles on MinGW, no?\n>\n> Yeah. I think a more clear way to phrase the question would be: is there\n> some trick we can use to booby-trap strcasecmp as a function pointer so\n> that it fails to compile even on systems where it would otherwise work?\n\nThat line of thought nudges us toward the place Linus explicitly\nsaid he didn't want to see us going, no? We do not particularly\nwant to care the exact nature of the breakage on MinGW. Do we\nreally want to set a booby-trap that intimately knows about how\ntheir strcasecmp is broken, and possibly cover breakages of the same\nkind but with other functions?\n\nIt isn't like \"we are deliberately relying on this non-standard\nbehaviour we see on the system _we_ commonly use, and somebody on\na new strictly POSIX platform may be bitten by it\", in which case it\nwould make sense to have a test that intimately knows about the\nnon-standard behaviour we rely on. This case is a total opposite.\n"},{"id":"227903","messageId":"20130919220531.GA13723@sigill.intra.peff.net","threadId":"34909","inReplyTo":"xmqqy56sqxj1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-19T22:05:31Z","receivedAt":"2013-09-19T22:05:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2013 at 03:03:46PM -0700, Junio C Hamano wrote:\n\n> >> But only when someone compiles on MinGW, no?\n> >\n> > Yeah. I think a more clear way to phrase the question would be: is there\n> > some trick we can use to booby-trap strcasecmp as a function pointer so\n> > that it fails to compile even on systems where it would otherwise work?\n> \n> That line of thought nudges us toward the place Linus explicitly\n> said he didn't want to see us going, no? We do not particularly\n> want to care the exact nature of the breakage on MinGW. Do we\n> really want to set a booby-trap that intimately knows about how\n> their strcasecmp is broken, and possibly cover breakages of the same\n> kind but with other functions?\n\nExactly. You snipped my second paragraph, but the gist of it was \"...and\nno, we do not want to go there\". Calling it a booby-trap was meant to be\nderogatory. :)\n\n-Peff\n"},{"id":"227905","messageId":"xmqqtxhgqvui.fsf@gitster.dls.corp.google.com","threadId":"34909","inReplyTo":"20130919220531.GA13723@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-19T22:40:05Z","receivedAt":"2013-09-19T22:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... \"...and\n> no, we do not want to go there\". Calling it a booby-trap was meant to be\n> derogatory. :)\n\nOK, I've resurrected the following and queued on 'pu'.\n\n-- >8 --\nSubject: [PATCH] mailmap: work around implementations with pure inline strcasecmp\n\nOn some systems (e.g. MinGW 4.0), string.h has only inline\ndefinition of strcasecmp and no non-inline implementation is\nsupplied anywhere, which is, eh, \"unusual\". We cannot take an\naddress of such a function to store it in namemap.cmp.\n\nWork it around by introducing our own level of indirection.\n\nSigned-off-by: Junio C Hamano \n---\n mailmap.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 44614fc..91a7532 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -52,6 +52,20 @@ static void free_mailmap_entry(void *p, const char *s)\n \tstring_list_clear_func(&me->namemap, free_mailmap_info);\n }\n \n+/*\n+ * On some systems (e.g. MinGW 4.0), string.h has _only_ inline\n+ * definition of strcasecmp and no non-inline implementation is\n+ * supplied anywhere, which is, eh, \"unusual\"; we cannot take an\n+ * address of such a function to store it in namemap.cmp. This is\n+ * here as a workaround---do not assign strcasecmp directly to\n+ * namemap.cmp until we know no systems that matter have such an\n+ * \"unusual\" string.h.\n+ */\n+static int namemap_cmp(const char *a, const char *b)\n+{\n+\treturn strcasecmp(a, b);\n+}\n+\n static void add_mapping(struct string_list *map,\n \t\t\tchar *new_name, char *new_email,\n \t\t\tchar *old_name, char *old_email)\n@@ -75,7 +89,7 @@ static void add_mapping(struct string_list *map,\n \t\titem = string_list_insert_at_index(map, index, old_email);\n \t\tme = xcalloc(1, sizeof(struct mailmap_entry));\n \t\tme->namemap.strdup_strings = 1;\n-\t\tme->namemap.cmp = strcasecmp;\n+\t\tme->namemap.cmp = namemap_cmp;\n \t\titem->util = me;\n \t}\n \n@@ -241,7 +255,7 @@ int read_mailmap(struct string_list *map, char **repo_abbrev)\n \tint err = 0;\n \n \tmap->strdup_strings = 1;\n-\tmap->cmp = strcasecmp;\n+\tmap->cmp = namemap_cmp;\n \n \tif (!git_mailmap_blob && is_bare_repository())\n \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n-- \n1.8.4-613-ge7dc249\n"},{"id":"227920","messageId":"20130920031850.GB15101@sigill.intra.peff.net","threadId":"34909","inReplyTo":"xmqqtxhgqvui.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-20T03:18:50Z","receivedAt":"2013-09-20T03:18:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2013 at 03:40:05PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... \"...and\n> > no, we do not want to go there\". Calling it a booby-trap was meant to be\n> > derogatory. :)\n> \n> OK, I've resurrected the following and queued on 'pu'.\n\nLooks good to me.\n\n-Peff\n"},{"id":"227928","messageId":"024f85fe-96e9-4201-8b3a-2e15c9da53e8@email.android.com","threadId":"34909","inReplyTo":"20130919211659.GB16556@sigill.intra.peff.net","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Piotr Krukowiecki","fromEmail":"piotr.krukowiecki@gmail.com","sentAt":"2013-09-20T06:21:04Z","receivedAt":"2013-09-20T06:21:04Z","isPatch":true,"sender":{"key":"piotr.krukowiecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3259959?v=4"},"body":"Jeff King napisał:\n>On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote:\n>\n>> >> > it still ends up as a single function call). The downside is\n>that it has\n>> >> > to be remembered at each site that uses strcasecmp, but we do\n>not use\n>> >> > pointers to standard library functions very often.\n>> >>\n>> >> Is it possible to add a test which fails if wrapper is not used?\n>> >\n>> > No test needed for this, as compilation or linkage will fail, I\n>think.\n>> \n>> But only when someone compiles on MinGW, no?\n>\n>Yeah. I think a more clear way to phrase the question would be: is\n>there\n>some trick we can use to booby-trap strcasecmp as a function pointer so\n>that it fails to compile even on systems where it would otherwise work?\n>\n>I can't think off-hand of a way to do so using preprocessor tricks, and\n>even if we could, I suspect the result would end up quite ugly. \n\nWhat I meant was: can we add a test (in t/) which greps git source code and fails if it finds strcasecmp string? \n\nIt could count number of strcasecmp and expect to find only 1 or exclude known location of the wrapper. \n\n\n-- \nPiotr Krukowiecki \n"},{"id":"228124","messageId":"20130924053230.GB5875@sigill.intra.peff.net","threadId":"34909","inReplyTo":"024f85fe-96e9-4201-8b3a-2e15c9da53e8@email.android.com","subject":"Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-24T05:32:30Z","receivedAt":"2013-09-24T05:32:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 20, 2013 at 08:21:04AM +0200, Piotr Krukowiecki wrote:\n\n> >I can't think off-hand of a way to do so using preprocessor tricks, and\n> >even if we could, I suspect the result would end up quite ugly. \n> \n> What I meant was: can we add a test (in t/) which greps git source\n> code and fails if it finds strcasecmp string?\n> \n> It could count number of strcasecmp and expect to find only 1 or\n> exclude known location of the wrapper.\n\nNo, because it is perfectly fine (and desirable) to use strcasecmp as a\nfunction, just not as a function pointer. Telling the difference would\ninvolve minor parsing of C.\n\nSo I think the least bad thing is to simply catch it in review, or by\ntesting on affected platforms.\n\n-Peff\n"}]}