Sebastian Schuberth | 11 Sep 18:06 2013
Picon

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

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

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

diff --git a/git-compat-util.h b/git-compat-util.h
index be1c494..664305c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
 <at>  <at>  -85,6 +85,13  <at>  <at> 
 #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 */
 <at>  <at>  -99,10 +106,6  <at>  <at> 
 #include <stddef.h>
 #include <stdlib.h>
 #include <stdarg.h>
(Continue reading)

Jonathan Nieder | 11 Sep 20:29 2013
Picon

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

Sebastian Schuberth wrote:

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

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

Curious,
Jonathan
Jeff King | 11 Sep 21:16 2013
Picon

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

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

> Sebastian Schuberth wrote:
> 
> > This is necessary so that read_mailmap() can obtain a pointer to the
> > function.
> 
> Hm, what platform has strcasecmp() as an inline function?  Is this
> allowed by POSIX?  Even if it isn't, should we perhaps just work
> around it by providing our own thin static function wrapper in
> mailmap.c?

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

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

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

(Continue reading)

Piotr Krukowiecki | 19 Sep 08:04 2013
Picon

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

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

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

--

-- 
Piotr Krukowiecki
Sebastian Schuberth | 11 Sep 21:59 2013
Picon

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

On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder <jrnieder <at> gmail.com> wrote:

>> This is necessary so that read_mailmap() can obtain a pointer to the
>> function.
>
> Hm, what platform has strcasecmp() as an inline function?  Is this
> allowed by POSIX?  Even if it isn't, should we perhaps just work
> around it by providing our own thin static function wrapper in
> mailmap.c?

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

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

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

--

-- 
Sebastian Schuberth
Jeff King | 11 Sep 23:41 2013
Picon

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

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

> On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder <jrnieder <at> gmail.com> wrote:
> 
> >> This is necessary so that read_mailmap() can obtain a pointer to the
> >> function.
> >
> > Hm, what platform has strcasecmp() as an inline function?  Is this
> > allowed by POSIX?  Even if it isn't, should we perhaps just work
> > around it by providing our own thin static function wrapper in
> > mailmap.c?
> 
> I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
> string.h contains the following code (see [1]):
> 
> #ifndef __NO_INLINE__
> __CRT_INLINE int __cdecl __MINGW_NOTHROW
> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
> {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
> #else
> #define strncasecmp _strnicmp
> #endif

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

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

(Continue reading)

Sebastian Schuberth | 12 Sep 11:36 2013
Picon

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

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

>> I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
>> string.h contains the following code (see [1]):
>>
>> #ifndef __NO_INLINE__
>> __CRT_INLINE int __cdecl __MINGW_NOTHROW
>> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
>> {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
>> #else
>> #define strncasecmp _strnicmp
>> #endif
>
> What is the error the compiler reports? Can it take the address of other

The error message of GCC 4.8.1 is:

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

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

> inline functions? For example, can it compile:
>
>     inline int foo(void) { return 5; }
>     extern int bar(int (*cb)(void));
>     int call(void) { return bar(foo); }
(Continue reading)

John Keeping | 12 Sep 12:14 2013
Picon

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

On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:
> > Just wondering if that is the root of the problem, or if maybe there is
> > something else subtle going on. Also, does __CRT_INLINE just turn into
> > "inline", or is there perhaps some other pre-processor magic going on?
> 
> This is the function definition from string.h after preprocessing:
> 
> extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))
> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
>   {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}

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

    http://clang.llvm.org/compatibility.html#inline
Junio C Hamano | 12 Sep 17:37 2013
Picon
Picon

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

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

> On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:
>> > Just wondering if that is the root of the problem, or if maybe there is
>> > something else subtle going on. Also, does __CRT_INLINE just turn into
>> > "inline", or is there perhaps some other pre-processor magic going on?
>> 
>> This is the function definition from string.h after preprocessing:
>> 
>> extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))
>> strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
>>   {return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
>
> I wonder if GCC has changed it's behaviour to more closely match C99.
> Clang as a compatibility article about this sort of issue:
>
>     http://clang.llvm.org/compatibility.html#inline

Interesting.  The ways the page suggests as fixes are

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

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

I agree with an earlier conclusion that defining our own wrapper
(Continue reading)

Jeff King | 12 Sep 20:20 2013
Picon

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

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

> > I wonder if GCC has changed it's behaviour to more closely match C99.
> > Clang as a compatibility article about this sort of issue:
> >
> >     http://clang.llvm.org/compatibility.html#inline
> 
> Interesting.  The ways the page suggests as fixes are
> 
>  - change it to a "statis inline";
>  - remove "inline" from the definition;
>  - provide an external (non-inline) def somewhere else;
>  - compile with gnu899 dialect.

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

-Peff
Junio C Hamano | 12 Sep 20:35 2013
Picon
Picon

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

Jeff King <peff <at> peff.net> writes:

> On Thu, Sep 12, 2013 at 08:37:08AM -0700, Junio C Hamano wrote:
>
>> > I wonder if GCC has changed it's behaviour to more closely match C99.
>> > Clang as a compatibility article about this sort of issue:
>> >
>> >     http://clang.llvm.org/compatibility.html#inline
>> 
>> Interesting.  The ways the page suggests as fixes are
>> 
>>  - change it to a "statis inline";
>>  - remove "inline" from the definition;
>>  - provide an external (non-inline) def somewhere else;
>>  - compile with gnu899 dialect.
>
> Right, option 3 seems perfectly reasonable to me, as we must be prepared
> to cope with a decision not to inline the function, and there has to be
> _some_ linked implementation. But shouldn't libc be providing an
> external, linkable strcasecmp in this case?

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

I think we would want something like below.

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

On some systems, string.h has _only_ inline definition of strcasecmp
(Continue reading)

Jonathan Nieder | 12 Sep 20:38 2013
Picon

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

Junio C Hamano wrote:

> I think we would want something like below.

Looks good to me, but

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

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

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

Likewise.

With or without that change,
Reviewed-by: Jonathan Nieder <jrnieder <at> gmail.com>
Sebastian Schuberth | 12 Sep 21:51 2013
Picon

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

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

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

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

--

-- 
Sebastian Schuberth
Junio C Hamano | 12 Sep 22:08 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

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

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

	#define strcasecmp _stricmp

or something?
Sebastian Schuberth | 13 Sep 14:33 2013
Picon

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

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

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

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

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

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

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

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

(Continue reading)

Junio C Hamano | 13 Sep 16:26 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

OK.

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

Sebastian Schuberth | 13 Sep 21:34 2013
Picon

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

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

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

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

--

-- 
Sebastian Schuberth
Jonathan Nieder | 12 Sep 23:36 2013
Picon

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

Sebastian Schuberth wrote:

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

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

	extern inline __attribute__((__gnu_inline__))

to suppress the non-inline function definition.

Jonathan
Jeff King | 12 Sep 21:00 2013
Picon

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

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

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

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

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

-Peff
Sebastian Schuberth | 12 Sep 21:46 2013
Picon

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

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

>> > I wonder if GCC has changed it's behaviour to more closely match C99.
>> > Clang as a compatibility article about this sort of issue:
>> >
>> >     http://clang.llvm.org/compatibility.html#inline
>>
>> Interesting.  The ways the page suggests as fixes are
>>
>>  - change it to a "statis inline";
>>  - remove "inline" from the definition;
>>  - provide an external (non-inline) def somewhere else;
>>  - compile with gnu899 dialect.
>
> Right, option 3 seems perfectly reasonable to me, as we must be prepared
> to cope with a decision not to inline the function, and there has to be
> _some_ linked implementation. But shouldn't libc be providing an
> external, linkable strcasecmp in this case?

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

Jeff King | 12 Sep 22:22 2013
Picon

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

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

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

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

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

I think there are basically three classes of solution:

  1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other
(Continue reading)

Junio C Hamano | 12 Sep 22:29 2013
Picon
Picon

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

Jeff King <peff <at> peff.net> writes:

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

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

Thanks, both.

(Continue reading)

Sebastian Schuberth | 13 Sep 14:47 2013
Picon

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

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

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

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

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

Junio C Hamano | 13 Sep 16:37 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

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

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
>  <at>  <at>  -85,12 +85,16  <at>  <at> 
>  #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
(Continue reading)

Sebastian Schuberth | 13 Sep 21:53 2013
Picon

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

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

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

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

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

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

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

+#ifdef __MINGW32__
+#ifdef __NO_INLINE__
+#define __NO_INLINE_ALREADY_DEFINED
+#else
(Continue reading)

Linus Torvalds | 13 Sep 21:56 2013

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

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

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

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

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

               Linus
Junio C Hamano | 13 Sep 22:01 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

Am I mistaken?

Sebastian Schuberth | 13 Sep 22:04 2013
Picon

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

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

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

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

--

-- 
Sebastian Schuberth
Junio C Hamano | 14 Sep 00:06 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

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

	#define __NO_INLINE__

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

(Continue reading)

Junio C Hamano | 14 Sep 00:35 2013
Picon
Picon

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

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

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

You need to have one level of quoting to keep "" from being eaten;
it should be sufficient to see how SHA1_HEADER that is included in
cache.h is handled and imitate it.
Sebastian Schuberth | 15 Sep 14:44 2013
Picon

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

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

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

Junio C Hamano | 17 Sep 18:17 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

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

Sebastian Schuberth | 17 Sep 21:16 2013
Picon

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

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

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

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

--

-- 
Sebastian Schuberth
Junio C Hamano | 17 Sep 23:46 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

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

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

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

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

Sebastian Schuberth | 18 Sep 11:43 2013
Picon

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

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

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

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

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

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

>> I do insist to avoid GCC-ism in C files,...
>
> To that I tend to agree.  Unconditionally killing inlining for any
(Continue reading)

Linus Torvalds | 18 Sep 14:19 2013

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

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

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

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

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

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

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

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

(Continue reading)

Jonathan Nieder | 12 Sep 23:31 2013
Picon

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

Sebastian Schuberth wrote:

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

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

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

Thanks,
Jonathan
Sebastian Schuberth | 19 Sep 15:47 2013
Picon

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

On 12.09.2013 23:31, Jonathan Nieder wrote:

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

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

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

--

-- 
Sebastian Schuberth
Junio C Hamano | 11 Sep 20:39 2013
Picon
Picon

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

Sebastian Schuberth <sschuberth <at> gmail.com> writes:

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

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

>  
> +#define __NO_INLINE__ /* do not inline strcasecmp() */
> +#include <string.h>
> +#ifdef HAVE_STRINGS_H
> +#include <strings.h> /* for strcasecmp() */
> +#endif
> +#undef __NO_INLINE__
> +
>  #ifdef WIN32 /* Both MinGW and MSVC */
>  #define _WIN32_WINNT 0x0502
>  #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
>  <at>  <at>  -99,10 +106,6  <at>  <at> 
>  #include <stddef.h>
>  #include <stdlib.h>
>  #include <stdarg.h>
> -#include <string.h>
> -#ifdef HAVE_STRINGS_H
> -#include <strings.h> /* for strcasecmp() */
> -#endif
(Continue reading)


Gmane