Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Fri, Sep 20, 2013 at 08:21:04AM +0200, Piotr Krukowiecki wrote: I can't think off-hand of a way to do so using preprocessor tricks, and even if we could, I suspect the result would end up quite ugly. What I meant was: can we add a test (in t/) which greps git source code and fails if it finds strcasecmp string? It could count number of strcasecmp and expect to find only 1 or exclude known location of the wrapper. No, because it is perfectly fine (and desirable) to use strcasecmp as a function, just not as a function pointer. Telling the difference would involve minor parsing of C. So I think the least bad thing is to simply catch it in review, or by testing on affected platforms. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Jeff King p...@peff.net napisał: On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote: it still ends up as a single function call). The downside is that it has to be remembered at each site that uses strcasecmp, but we do not use pointers to standard library functions very often. Is it possible to add a test which fails if wrapper is not used? No test needed for this, as compilation or linkage will fail, I think. But only when someone compiles on MinGW, no? Yeah. I think a more clear way to phrase the question would be: is there some trick we can use to booby-trap strcasecmp as a function pointer so that it fails to compile even on systems where it would otherwise work? I can't think off-hand of a way to do so using preprocessor tricks, and even if we could, I suspect the result would end up quite ugly. What I meant was: can we add a test (in t/) which greps git source code and fails if it finds strcasecmp string? It could count number of strcasecmp and expect to find only 1 or exclude known location of the wrapper. -- Piotr Krukowiecki -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Wed, Sep 11, 2013 at 9:16 PM, Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 19, 2013 at 9:37 AM, Junio C Hamano ju...@pobox.com wrote: On Sep 18, 2013 11:08 PM, Piotr Krukowiecki piotr.krukowie...@gmail.com wrote: On Wed, Sep 11, 2013 at 9:16 PM, Jeff King p...@peff.net wrote: I would prefer the static wrapper solution you suggest, though. It [...] it still ends up as a single function call). The downside is that it has to be remembered at each site that uses strcasecmp, but we do not use pointers to standard library functions very often. Is it possible to add a test which fails if wrapper is not used? No test needed for this, as compilation or linkage will fail, I think. But only when someone compiles on MinGW, no? -- Piotr Krukowiecki -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote: it still ends up as a single function call). The downside is that it has to be remembered at each site that uses strcasecmp, but we do not use pointers to standard library functions very often. Is it possible to add a test which fails if wrapper is not used? No test needed for this, as compilation or linkage will fail, I think. But only when someone compiles on MinGW, no? Yeah. I think a more clear way to phrase the question would be: is there some trick we can use to booby-trap strcasecmp as a function pointer so that it fails to compile even on systems where it would otherwise work? I can't think off-hand of a way to do so using preprocessor tricks, and even if we could, I suspect the result would end up quite ugly. It's probably enough to just catch such problems in review, or let people on affected systems report and fix the error if it slips through. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Jeff King p...@peff.net writes: On Thu, Sep 19, 2013 at 11:47:51AM +0200, Piotr Krukowiecki wrote: it still ends up as a single function call). The downside is that it has to be remembered at each site that uses strcasecmp, but we do not use pointers to standard library functions very often. Is it possible to add a test which fails if wrapper is not used? No test needed for this, as compilation or linkage will fail, I think. But only when someone compiles on MinGW, no? Yeah. I think a more clear way to phrase the question would be: is there some trick we can use to booby-trap strcasecmp as a function pointer so that it fails to compile even on systems where it would otherwise work? That line of thought nudges us toward the place Linus explicitly said he didn't want to see us going, no? We do not particularly want to care the exact nature of the breakage on MinGW. Do we really want to set a booby-trap that intimately knows about how their strcasecmp is broken, and possibly cover breakages of the same kind but with other functions? It isn't like we are deliberately relying on this non-standard behaviour we see on the system _we_ commonly use, and somebody on a new strictly POSIX platform may be bitten by it, in which case it would make sense to have a test that intimately knows about the non-standard behaviour we rely on. This case is a total opposite. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 19, 2013 at 03:03:46PM -0700, Junio C Hamano wrote: But only when someone compiles on MinGW, no? Yeah. I think a more clear way to phrase the question would be: is there some trick we can use to booby-trap strcasecmp as a function pointer so that it fails to compile even on systems where it would otherwise work? That line of thought nudges us toward the place Linus explicitly said he didn't want to see us going, no? We do not particularly want to care the exact nature of the breakage on MinGW. Do we really want to set a booby-trap that intimately knows about how their strcasecmp is broken, and possibly cover breakages of the same kind but with other functions? Exactly. You snipped my second paragraph, but the gist of it was ...and no, we do not want to go there. Calling it a booby-trap was meant to be derogatory. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Jeff King p...@peff.net writes:
... ...and
no, we do not want to go there. Calling it a booby-trap was meant to be
derogatory. :)
OK, I've resurrected the following and queued on 'pu'.
-- 8 --
Subject: [PATCH] mailmap: work around implementations with pure inline
strcasecmp
On some systems (e.g. MinGW 4.0), string.h has only inline
definition of strcasecmp and no non-inline implementation is
supplied anywhere, which is, eh, unusual. We cannot take an
address of such a function to store it in namemap.cmp.
Work it around by introducing our own level of indirection.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
mailmap.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index 44614fc..91a7532 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -52,6 +52,20 @@ static void free_mailmap_entry(void *p, const char *s)
string_list_clear_func(me-namemap, free_mailmap_info);
}
+/*
+ * On some systems (e.g. MinGW 4.0), string.h has _only_ inline
+ * definition of strcasecmp and no non-inline implementation is
+ * supplied anywhere, which is, eh, unusual; we cannot take an
+ * address of such a function to store it in namemap.cmp. This is
+ * here as a workaround---do not assign strcasecmp directly to
+ * namemap.cmp until we know no systems that matter have such an
+ * unusual string.h.
+ */
+static int namemap_cmp(const char *a, const char *b)
+{
+ return strcasecmp(a, b);
+}
+
static void add_mapping(struct string_list *map,
char *new_name, char *new_email,
char *old_name, char *old_email)
@@ -75,7 +89,7 @@ static void add_mapping(struct string_list *map,
item = string_list_insert_at_index(map, index, old_email);
me = xcalloc(1, sizeof(struct mailmap_entry));
me-namemap.strdup_strings = 1;
- me-namemap.cmp = strcasecmp;
+ me-namemap.cmp = namemap_cmp;
item-util = me;
}
@@ -241,7 +255,7 @@ int read_mailmap(struct string_list *map, char
**repo_abbrev)
int err = 0;
map-strdup_strings = 1;
- map-cmp = strcasecmp;
+ map-cmp = namemap_cmp;
if (!git_mailmap_blob is_bare_repository())
git_mailmap_blob = HEAD:.mailmap;
--
1.8.4-613-ge7dc249
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 19, 2013 at 03:40:05PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... ...and no, we do not want to go there. Calling it a booby-trap was meant to be derogatory. :) OK, I've resurrected the following and queued on 'pu'. Looks good to me. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Tue, Sep 17, 2013 at 11:46 PM, Junio C Hamano gits...@pobox.com wrote: I don't think people on other platforms seeing the ugliness is really an issue. After all, the file is called git-*compat*-util.h; Well, judging from the way Linus reacted to the patch, I'd have to disagree. After all, that argument leads to the position that nothing is needed in compat/, no? My feeling is that Linus' reaction was more about that this work-around is even necessary (and MinGW is buggy) rather than applying it to git-compat-util.h and not elsewhere. One ugliness (lack of sane strcasecmp definition whose address can be taken) specific to mingw is worked around in compat/mingw.h, and another ugliness that some people may use compilers without include_next may need help from another configuration in the Makefile to tell it where the platform string.h resides. I am not sure why you see it as a problem. I just don't like that the ugliness is spreading out and requires a change to config.mak.uname now, too. Also, I regard the change to config.mak.uname by itself as ugly, mainly because you would have to set SYSTEM_STRING_H_HEADER to some path, but that path might differ from system to system, depending on where MinGW is installed on Windows. I do insist to avoid GCC-ism in C files,... To that I tend to agree. Unconditionally killing inlining for any mingw compilation in compat/mingw.h may be the simplest (albeit it may be less than optimal) solution. I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed, it involved the need to shuffle includes in git-compat-util.h around because winsock2.h already seems to include string.h, and I did not find a working include order. So I came up with the following, do you like that better? diff --git a/compat/string_no_inline.h b/compat/string_no_inline.h new file mode 100644 index 000..51eed52 --- /dev/null +++ b/compat/string_no_inline.h @@ -0,0 +1,25 @@ +#ifndef STRING_NO_INLINE_H +#define STRING_NO_INLINE_H + +#ifdef __MINGW32__ +#ifdef __NO_INLINE__ +#define __NO_INLINE_ALREADY_DEFINED +#else +#define __NO_INLINE__ /* do not inline strcasecmp() */ +#endif +#endif + +#include string.h +#ifdef HAVE_STRINGS_H +#include strings.h /* for strcasecmp() */ +#endif + +#ifdef __MINGW32__ +#ifdef __NO_INLINE_ALREADY_DEFINED +#undef __NO_INLINE_ALREADY_DEFINED +#else +#undef __NO_INLINE__ +#endif +#endif + +#endif diff --git a/git-compat-util.h b/git-compat-util.h index db564b7..348dd55 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -85,6 +85,8 @@ #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#include compat/string_no_inline.h + #ifdef WIN32 /* Both MinGW and MSVC */ #ifndef _WIN32_WINNT #define _WIN32_WINNT 0x0502 @@ -101,10 +103,6 @@ #include stddef.h #include stdlib.h #include stdarg.h -#include string.h -#ifdef HAVE_STRINGS_H -#include strings.h /* for strcasecmp() */ -#endif #include errno.h #include limits.h #ifdef NEEDS_SYS_PARAM_H -- 1.8.3.mingw.1.dirty -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Wed, Sep 18, 2013 at 5:43 AM, Sebastian Schuberth sschube...@gmail.com wrote: My feeling is that Linus' reaction was more about that this work-around is even necessary (and MinGW is buggy) rather than applying it to git-compat-util.h and not elsewhere. So I think it's an annoying MinGW bug, but the reason I dislike the no-inline approach is two-fold: - it's *way* too intimate with the bug. When you have a bug like this, the *last* thing you want to do is to make sweet sweet love to it, and get really involved with it. You want to say Eww, what a nasty little bug, I don't want to have anything to do with you. And quite frankly, delving into the details of exactly *what* MinGW does wrong, and defining magic __NO_INLINE__ macros, knowing that that is the particular incantation that hides the MinGW bug, that's being too intimate. That's simply a level of detail that *nobody* should ever have to know. The other patch (having just a wrapper function) doesn't have those kinds of intimacy issues. That patch just says MinGW is buggy and cannot do this function uninlined, so we wrap it. Notice the lack of detail, and lack of *interest* in the exact particular pattern of the bug. The other reason I'm not a fan of the __NO_INLINE__ approach is even more straightforward: - Why should we disable the inlining of everything in string.h (and possibly elsewhere too - who the hell knows what __NO_INLINE__ will do to other header files), when in 99% of all the cases we don't care, and in fact inlining may well be good and the right thing to do. So the __NO_INLINE__ games seem to be both too big of a hammer, and too non-specific, and at the same time it gets really intimate with MinGW in unhealthy ways. If you know something is diseased, you keep your distance, you don't try to embrace it. I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed, it involved the need to shuffle includes in git-compat-util.h around because winsock2.h already seems to include string.h, and I did not find a working include order. So I came up with the following, do you like that better? Ugh, so now that patch is fragile, so we have to complicate it even more. Really, just make a wrapper function. It doesn't even need to be conditional on MinGW. Just a single one-liner function, with a comment above it that says MinGW is broken and doesn't have an out-of-line copy of strcasecmp(), so we wrap it here. No unnecessary details about internal workings of a buggy MinGW header file. No complexity. No subtle issues with include file ordering. Just a straightforward workaround that is easy to explain. Linus -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano gits...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@gmail.com writes: On Tue, Sep 17, 2013 at 6:17 PM, Junio C Hamano gits...@pobox.com wrote: Keeping the ugliness to deal with the platform issue (i.e. broken string.h) in one place (e.g. compat/mingw) is far more preferrable than having a similar ugliness in git-compat-util.h for people on all other platforms to see, no? I don't think people on other platforms seeing the ugliness is really an issue. After all, the file is called git-*compat*-util.h; Well, judging from the way Linus reacted to the patch, I'd have to disagree. After all, that argument leads to the position that nothing is needed in compat/, no? Also, your solution does not really keep the ugliness in one place,... One ugliness (lack of sane strcasecmp definition whose address can be taken) specific to mingw is worked around in compat/mingw.h, and another ugliness that some people may use compilers without include_next may need help from another configuration in the Makefile to tell it where the platform string.h resides. I am not sure why you see it as a problem. I do insist to avoid GCC-ism in C files,... To that I tend to agree. Unconditionally killing inlining for any mingw compilation in compat/mingw.h may be the simplest (albeit it may be less than optimal) solution. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Sat, Sep 14, 2013 at 12:06 AM, Junio C Hamano gits...@pobox.com wrote: You can explicitly include the system header from your compatibility layer, i.e. === compat/mingw/string.h === #define __NO_INLINE__ #ifdef SYSTEM_STRING_H_HEADER #include SYSTEM_STRING_H_HEADER #else #include_next string.h #endif and then in config.mak.uname, do something like this: ifneq (,$(findstring MINGW,$(uname_S))) ifndef SYSTEM_STRING_H_HEADER SYSTEM_STRING_H_HEADER = C:\\llvm\include\string.h endif COMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER) endif People who have the system header file at different paths can further override SYSTEM_STRING_H_HEADER in their config.mak. That would help compilers targetting mingw that do not support #include_next without spreading the damage to other people's systems, I think. I think this is less favorable compared to my last proposed solution. While my work-around in git-compat-util.h from [1] already is quite ugly, it's at least in a single place. You solution spreads the code it multiple place, making it even more ugly and less comprehensible, IMHO. [1] http://www.spinics.net/lists/git/msg217546.html -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 12, 2013 at 10:08 PM, Junio C Hamano gits...@pobox.com wrote: I'm not too happy with the wording either. As I see it, even on MinGW runtime version 4.0 it's not true that string.h has _only_ inline definition of strcasecmp; there's also #define strncasecmp _strnicmp which effectively provides a non-inline definition of strncasecmp aka _strnicmp. I do not get this part. Sure, string.h would have definitions of things other than strcasecmp, such as strncasecmp. So what? Sorry, I mixed up strcasecmp and strncasecmp. Does it effectively provide a non-inline definition of strcasecmp? Yes, if __NO_INLINE__ is defined string.h provides non-inline definition of both strcasecmp and strncasecmp by defining them to _stricmp and _strnicmp respectively. Perhaps the real issue is that the header file does not give an equivalent those who want to take the address of strcasecmp will get the address of _stricmp instead macro, e.g. #define strcasecmp _stricmp or something? Now it's you who puzzles me, because the header file *does* have exactly the macro that you suggest. Anyway, I think Peff's reply to my other mail summed it up nicely. I will come up with another patch. -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 12, 2013 at 10:29 PM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: I think there are basically three classes of solution: 1. Declare __NO_INLINE__ everywhere. I'd worry this might affect other environments, who would then not inline and lose performance (but since it's a non-standard macro, we don't really know what it will do in other places; possibly nothing). 2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it only affects mingw, and we know the meaning of NO_INLINE there. 3. Try to impact only the uses as a function pointer (e.g., by using a wrapper function as suggested in the thread). Your patch does (1), I believe. Junio's patch does (3), but is a maintenance burden in that any new callsites will need to remember to do the same trick. Well, if by everywhere in (1) you mean on all platforms then you're right. But my patch does not define __NO_INLINE__ globally, but only at the time string.h / strings.h is included. Afterwards __NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not defined everywhere. Agreed. If that #define __NO_INLINE__ does not appear in the common part of our header files like git-compat-util.h but is limited to somewhere in compat/, that would be the perfect outcome. It's not that easy to move the definition of __NO_INLINE__ into compat/ because git-compat-util.h includes string.h / strings.h before anything of compat/. More over, defining __NO_INLINE__ in somewhere in compat/ would not limit its definition to the string.h / strings.h headers only. So how about something like this on top of my original patch: --- a/git-compat-util.h +++ b/git-compat-util.h @@ -85,12 +85,16 @@ #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#ifdef __MINGW32__ #define __NO_INLINE__ /* do not inline strcasecmp() */ +#endif #include string.h +#ifdef __MINGW32__ +#undef __NO_INLINE__ +#endif #ifdef HAVE_STRINGS_H #include strings.h /* for strcasecmp() */ #endif -#undef __NO_INLINE__ #ifdef WIN32 /* Both MinGW and MSVC */ #ifndef _WIN32_WINNT -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@gmail.com writes: On Thu, Sep 12, 2013 at 10:08 PM, Junio C Hamano gits...@pobox.com wrote: I'm not too happy with the wording either. As I see it, even on MinGW runtime version 4.0 it's not true that string.h has _only_ inline definition of strcasecmp; there's also #define strncasecmp _strnicmp which effectively provides a non-inline definition of strncasecmp aka _strnicmp. I do not get this part. Sure, string.h would have definitions of things other than strcasecmp, such as strncasecmp. So what? Sorry, I mixed up strcasecmp and strncasecmp. OK. Does it effectively provide a non-inline definition of strcasecmp? Yes, if __NO_INLINE__ is defined string.h provides non-inline definition of both strcasecmp and strncasecmp by defining them to _stricmp and _strnicmp respectively. Perhaps the real issue is that the header file does not give an equivalent those who want to take the address of strcasecmp will get the address of _stricmp instead macro, e.g. #define strcasecmp _stricmp or something? Now it's you who puzzles me, because the header file *does* have exactly the macro that you suggest. Then why does your platform have problem with the code that takes the address of strcasecmp and stores it in the variable? It is not me, but your platform that is puzzling us. There is something else going on, like you do not have that #define enabled under some condition, or something silly like that. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@gmail.com writes: Well, if by everywhere in (1) you mean on all platforms then you're right. But my patch does not define __NO_INLINE__ globally, but only at the time string.h / strings.h is included. Afterwards __NO_INLINE__ is undefined. In that sense, __NO_INLINE__ is not defined everywhere. Which means people who do want to see that macro defined will be broken after that section of the header file which unconditionally undefs it, right? That is exactly why that change should not appear in the platform neutral part of the header file. --- a/git-compat-util.h +++ b/git-compat-util.h @@ -85,12 +85,16 @@ #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#ifdef __MINGW32__ #define __NO_INLINE__ /* do not inline strcasecmp() */ +#endif #include string.h +#ifdef __MINGW32__ +#undef __NO_INLINE__ +#endif That is certainly better than the unconditional one, but I wonder if it is an option to add compat/mingw/string.h without doing the above, though. That header file can do the no-inline dance before including the real thing with #include_next, and nobody else would notice, no? #ifdef __NO_INLINE__ #define __NO_INLINE_WAS_THERE 1 #else #define __NO_INLINE__ #define __NO_INLINE_WAS_THERE 0 #endif #include_next string.h #if !__NO_INLINE_WAS_THERE #undef __NO_INLINE__ #endif or something like that. That of course assumes nobody compiles for _MINGW32_ with a compiler that does not understrand #include_next and I do not know if that restriction is a showstopper or not. #ifdef HAVE_STRINGS_H #include strings.h /* for strcasecmp() */ #endif -#undef __NO_INLINE__ #ifdef WIN32 /* Both MinGW and MSVC */ #ifndef _WIN32_WINNT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Fri, Sep 13, 2013 at 4:26 PM, Junio C Hamano gits...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Fri, Sep 13, 2013 at 4:37 PM, Junio C Hamano gits...@pobox.com wrote: Which means people who do want to see that macro defined will be broken after that section of the header file which unconditionally undefs it, right? Right, but luckily you've fixed that in our proposed patch :-) That is certainly better than the unconditional one, but I wonder if it is an option to add compat/mingw/string.h without doing the above, though. I don't like the idea of introducing a compat/mingw/string.h because of two reasons: You would have to add a conditional to include that string.h instead of the system one anyway, so we could just as well keep the conditional in git-compat-util.h along with the logic. And I don't like the include_next GCC-ism, especially as I was planning to take a look at compiling Git with LLVM / clang under Windows. So how about this: --- a/git-compat-util.h +++ b/git-compat-util.h @@ -85,6 +85,25 @@ #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#ifdef __MINGW32__ +#ifdef __NO_INLINE__ +#define __NO_INLINE_ALREADY_DEFINED +#else +#define __NO_INLINE__ /* do not inline strcasecmp() */ +#endif +#endif +#include string.h +#ifdef __MINGW32__ +#ifdef __NO_INLINE_ALREADY_DEFINED +#undef __NO_INLINE_ALREADY_DEFINED +#else +#undef __NO_INLINE__ +#endif +#endif +#ifdef HAVE_STRINGS_H +#include strings.h /* for strcasecmp() */ +#endif + #ifdef WIN32 /* Both MinGW and MSVC */ #define _WIN32_WINNT 0x0502 #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */ @@ -99,10 +118,6 @@ #include stddef.h #include stdlib.h #include stdarg.h -#include string.h -#ifdef HAVE_STRINGS_H -#include strings.h /* for strcasecmp() */ -#endif #include errno.h #include limits.h #ifdef NEEDS_SYS_PARAM_H -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Fri, Sep 13, 2013 at 12:53 PM, Sebastian Schuberth sschube...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Fri, Sep 13, 2013 at 9:56 PM, Linus Torvalds torva...@linux-foundation.org wrote: +#ifdef __MINGW32__ +#ifdef __NO_INLINE__ Why do you want to push this insane workaround for a clear Mingw bug? To be frank, because Git is picking up patches much quicker than MinGW does, and I want a solution ASAP. Although I of course agree that fixing the real issue upstream in MinGW is the better solution. Please have mingw just fix the nasty bug, and the git patch with the I'll try to come up with a MinGW patch in parallel. trivial wrapper looks much simpler than just saying don't inline anything and that crazy block of nasty mingw magic #defines/. It may look simpler, but as outlines in this thread it's less maintainable because you need to remember to use the wrapper. And people tend to forget that no matter how loudly you document that. If we can make the code more fool proof we IMHO should do so. -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Fri, Sep 13, 2013 at 10:01 PM, Junio C Hamano gits...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@gmail.com writes: On Fri, Sep 13, 2013 at 10:01 PM, Junio C Hamano gits...@pobox.com wrote: I don't like the idea of introducing a compat/mingw/string.h because of two reasons: You would have to add a conditional to include that string.h instead of the system one anyway, With -Icompat/mingw passed to the compiler, which is a bog-standard technique we already use to supply headers the system forgot to supply or override buggy headers the system is shipped with, you do not have to change any #include string.h. Am I mistaken? Ah, that would work I guess, but you'd still need the include_next. You can explicitly include the system header from your compatibility layer, i.e. === compat/mingw/string.h === #define __NO_INLINE__ #ifdef SYSTEM_STRING_H_HEADER #include SYSTEM_STRING_H_HEADER #else #include_next string.h #endif and then in config.mak.uname, do something like this: ifneq (,$(findstring MINGW,$(uname_S))) ifndef SYSTEM_STRING_H_HEADER SYSTEM_STRING_H_HEADER = C:\\llvm\include\string.h endif COMPAT_CFLAGS += -DSYSTEM_STRING_H_HEADER=$(SYSTEM_STRING_H_HEADER) endif People who have the system header file at different paths can further override SYSTEM_STRING_H_HEADER in their config.mak. That would help compilers targetting mingw that do not support #include_next without spreading the damage to other people's systems, I think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Wed, Sep 11, 2013 at 11:41 PM, Jeff King p...@peff.net wrote:
I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
string.h contains the following code (see [1]):
#ifndef __NO_INLINE__
__CRT_INLINE int __cdecl __MINGW_NOTHROW
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
#else
#define strncasecmp _strnicmp
#endif
What is the error the compiler reports? Can it take the address of other
The error message of GCC 4.8.1 is:
LINK git-credential-store.exe
libgit.a(mailmap.o): In function `read_mailmap':
C:\mingwGitDevEnv\git/mailmap.c:238: undefined reference to `strcasecmp'
collect2.exe: error: ld returned 1 exit status
make: *** [git-credential-store.exe] Error 1
So it's a linker error, not a compiler error.
inline functions? For example, can it compile:
inline int foo(void) { return 5; }
extern int bar(int (*cb)(void));
int call(void) { return bar(foo); }
I had to modify the example slightly to:
inline int foo(void) { return 5; }
extern int bar(int (*cb)(void)) { return cb(); }
int main(void) { return bar(foo); }
And this compiles.
Just wondering if that is the root of the problem, or if maybe there is
something else subtle going on. Also, does __CRT_INLINE just turn into
inline, or is there perhaps some other pre-processor magic going on?
This is the function definition from string.h after preprocessing:
extern __inline__ int __attribute__((__cdecl__)) __attribute__ ((__nothrow__))
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
--
Sebastian Schuberth
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
John Keeping j...@keeping.me.uk writes:
On Thu, Sep 12, 2013 at 11:36:56AM +0200, Sebastian Schuberth wrote:
Just wondering if that is the root of the problem, or if maybe there is
something else subtle going on. Also, does __CRT_INLINE just turn into
inline, or is there perhaps some other pre-processor magic going on?
This is the function definition from string.h after preprocessing:
extern __inline__ int __attribute__((__cdecl__)) __attribute__
((__nothrow__))
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
I wonder if GCC has changed it's behaviour to more closely match C99.
Clang as a compatibility article about this sort of issue:
http://clang.llvm.org/compatibility.html#inline
Interesting. The ways the page suggests as fixes are
- change it to a statis inline;
- remove inline from the definition;
- provide an external (non-inline) def somewhere else;
- compile with gnu899 dialect.
But the first two are non-starter, and the third one to force
everybody to define an equivalent implementation is nonsense, for a
definition in the standard header file.
I agree with an earlier conclusion that defining our own wrapper
(with an explanation why such a redundant wrapper exists) is the
best course of action at this point, until the system header is
fixed.
mailmap.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index a7969c4..d36d424 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)
string_list_clear_func(me-namemap, free_mailmap_info);
}
+/*
+ * On some systems, string.h has _only_ inline definition of strcasecmp
+ * without supplying a non-inline implementation anywhere, which is, eh,
+ * unusual; we cannot take an address of such a function to store it in
+ * namemap.cmp. This is here as a workaround---do not assign strcasecmp
+ * directly to namemap.cmp until we know no systems that matter have such
+ * an unusual string.h.
+ */
+static int namemap_cmp(const char *a, const char *b)
+{
+ return strcasecmp(a, b);
+}
+
static void add_mapping(struct string_list *map,
char *new_name, char *new_email,
char *old_name, char *old_email)
@@ -75,7 +88,7 @@ static void add_mapping(struct string_list *map,
item = string_list_insert_at_index(map, index, old_email);
me = xcalloc(1, sizeof(struct mailmap_entry));
me-namemap.strdup_strings = 1;
- me-namemap.cmp = strcasecmp;
+ me-namemap.cmp = namemap_cmp;
item-util = me;
}
@@ -237,7 +250,7 @@ int read_mailmap(struct string_list *map, char
**repo_abbrev)
int err = 0;
map-strdup_strings = 1;
- map-cmp = strcasecmp;
+ map-cmp = namemap_cmp;
if (!git_mailmap_blob is_bare_repository())
git_mailmap_blob = HEAD:.mailmap;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 @@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s) string_list_clear_func(me-namemap, free_mailmap_info); } +/* + * On some systems, string.h has _only_ inline definition of strcasecmp Likewise. With or without that change, Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Jeff King p...@peff.net writes:
On Thu, Sep 12, 2013 at 08:37:08AM -0700, Junio C Hamano wrote:
I wonder if GCC has changed it's behaviour to more closely match C99.
Clang as a compatibility article about this sort of issue:
http://clang.llvm.org/compatibility.html#inline
Interesting. The ways the page suggests as fixes are
- change it to a statis inline;
- remove inline from the definition;
- provide an external (non-inline) def somewhere else;
- compile with gnu899 dialect.
Right, option 3 seems perfectly reasonable to me, as we must be prepared
to cope with a decision not to inline the function, and there has to be
_some_ linked implementation. But shouldn't libc be providing an
external, linkable strcasecmp in this case?
That is exactly my point when I said that the third one is nonsense
for a definition in the standard header file.
I think we would want something like below.
-- 8 --
Subject: [PATCH] mailmap: work around implementations with pure inline
strcasecmp
On some systems, string.h has _only_ inline definition of strcasecmp
without supplying a non-inline implementation anywhere, which is,
eh, unusual; we cannot take an address of such a function to store
it in namemap.cmp. Work it around by introducing our own level of
indirection.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
mailmap.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index 44614fc..8863e23 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -52,6 +52,19 @@ static void free_mailmap_entry(void *p, const char *s)
string_list_clear_func(me-namemap, free_mailmap_info);
}
+/*
+ * On some systems, string.h has _only_ inline definition of strcasecmp
+ * without supplying a non-inline implementation anywhere, which is, eh,
+ * unusual; we cannot take an address of such a function to store it in
+ * namemap.cmp. This is here as a workaround---do not assign strcasecmp
+ * directly to namemap.cmp until we know no systems that matter have such
+ * an unusual string.h.
+ */
+static int namemap_cmp(const char *a, const char *b)
+{
+ return strcasecmp(a, b);
+}
+
static void add_mapping(struct string_list *map,
char *new_name, char *new_email,
char *old_name, char *old_email)
@@ -75,7 +88,7 @@ static void add_mapping(struct string_list *map,
item = string_list_insert_at_index(map, index, old_email);
me = xcalloc(1, sizeof(struct mailmap_entry));
me-namemap.strdup_strings = 1;
- me-namemap.cmp = strcasecmp;
+ me-namemap.cmp = namemap_cmp;
item-util = me;
}
@@ -241,7 +254,7 @@ int read_mailmap(struct string_list *map, char
**repo_abbrev)
int err = 0;
map-strdup_strings = 1;
- map-cmp = strcasecmp;
+ map-cmp = namemap_cmp;
if (!git_mailmap_blob is_bare_repository())
git_mailmap_blob = HEAD:.mailmap;
--
1.8.4-485-gec42fe2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Thu, Sep 12, 2013 at 8:20 PM, Jeff King p...@peff.net wrote: I wonder if GCC has changed it's behaviour to more closely match C99. Clang as a compatibility article about this sort of issue: http://clang.llvm.org/compatibility.html#inline Interesting. The ways the page suggests as fixes are - change it to a statis inline; - remove inline from the definition; - provide an external (non-inline) def somewhere else; - compile with gnu899 dialect. Right, option 3 seems perfectly reasonable to me, as we must be prepared to cope with a decision not to inline the function, and there has to be _some_ linked implementation. But shouldn't libc be providing an external, linkable strcasecmp in this case? MinGW / GCC is not linking against libc, but against MSVCRT, Visual Studio's C runtime. And in fact MSVCRT has a non-inline implementation of a case-insensitive string comparison for up to the first n characters; it just happens to be called _strnicmp, not strncasecmp. Which is why I still think just having a #define strncasecmp _strnicmp is the most elegant solution to the problem. And that's exactly what defining __NO_INLINE__ does. Granted, defining __NO_INLINE__ in the scope of string.h will also add a #define strcasecmp _stricmp; but despite it's name, defining __NO_INLINE__ does not imply a performance hit due to functions not being inlined because it's just the strncasecmp wrapper around _strnicmp that's being inlined, not _strnicmp itself. -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 environments, who would then not inline and lose performance (but since it's a non-standard macro, we don't really know what it will do in other places; possibly nothing). 2. Declare __NO_INLINE__ on mingw. Similar to above, but we know it only affects mingw, and we know the meaning of NO_INLINE there. 3. Try to impact only the uses as a function pointer (e.g., by using a wrapper function as suggested in the thread). Your patch does (1), I believe. Junio's patch does (3), but is a maintenance burden in that any new callsites will need to remember to do the same trick. But your argument (and reading the mingw header, I agree) is that there is no performance difference at all between (2) and (3). And (2) does not have the maintenance burden. So it does seem like the right path to me. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Jeff King p...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 sschube...@gmail.com --- git-compat-util.h | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index be1c494..664305c 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -85,6 +85,13 @@ #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#define __NO_INLINE__ /* do not inline strcasecmp() */ +#include string.h +#ifdef HAVE_STRINGS_H +#include strings.h /* for strcasecmp() */ +#endif +#undef __NO_INLINE__ + #ifdef WIN32 /* Both MinGW and MSVC */ #define _WIN32_WINNT 0x0502 #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */ @@ -99,10 +106,6 @@ #include stddef.h #include stdlib.h #include stdarg.h -#include string.h -#ifdef HAVE_STRINGS_H -#include strings.h /* for strcasecmp() */ -#endif #include errno.h #include limits.h #ifdef NEEDS_SYS_PARAM_H -- 1.8.3.mingw.1.2.g56240b5.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
Sebastian Schuberth sschube...@gmail.com writes: This is necessary so that read_mailmap() can obtain a pointer to the function. Whoa, I didn't think it is even legal for a C library to supply strcmp() or strcasecmp() that are purely inline you cannot take the address of. The solution looks a bit too large a hammer that affects everybody, not just those who have such a set of header files. +#define __NO_INLINE__ /* do not inline strcasecmp() */ +#include string.h +#ifdef HAVE_STRINGS_H +#include strings.h /* for strcasecmp() */ +#endif +#undef __NO_INLINE__ + #ifdef WIN32 /* Both MinGW and MSVC */ #define _WIN32_WINNT 0x0502 #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */ @@ -99,10 +106,6 @@ #include stddef.h #include stdlib.h #include stdarg.h -#include string.h -#ifdef HAVE_STRINGS_H -#include strings.h /* for strcasecmp() */ -#endif #include errno.h #include limits.h #ifdef NEEDS_SYS_PARAM_H -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-compat-util: Avoid strcasecmp() being inlined
On Wed, Sep 11, 2013 at 8:29 PM, Jonathan Nieder jrnie...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 jrnie...@gmail.com wrote:
This is necessary so that read_mailmap() can obtain a pointer to the
function.
Hm, what platform has strcasecmp() as an inline function? Is this
allowed by POSIX? Even if it isn't, should we perhaps just work
around it by providing our own thin static function wrapper in
mailmap.c?
I'm on Windows using MSYS / MinGW. Since MinGW runtime version 4.0,
string.h contains the following code (see [1]):
#ifndef __NO_INLINE__
__CRT_INLINE int __cdecl __MINGW_NOTHROW
strncasecmp (const char * __sz1, const char * __sz2, size_t __sizeMaxCompare)
{return _strnicmp (__sz1, __sz2, __sizeMaxCompare);}
#else
#define strncasecmp _strnicmp
#endif
What is the error the compiler reports? Can it take the address of other
inline functions? For example, can it compile:
inline int foo(void) { return 5; }
extern int bar(int (*cb)(void));
int call(void) { return bar(foo); }
Just wondering if that is the root of the problem, or if maybe there is
something else subtle going on. Also, does __CRT_INLINE just turn into
inline, or is there perhaps some other pre-processor magic going on?
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
