Re: [PATCH 17/19] Portable alloca for Git
Erik Faye-Lund writes: >> Subject: [PATCH] mingw: activate alloca >> >> Both MSVC and MINGW have alloca(3) definitions in malloc.h, so by moving >> win32-compat alloca.h from compat/vcbuild/include/ to compat/win32/ , >> which is included by both MSVC and MINGW CFLAGS, we can make alloca() >> work on both those Windows environments. >> >> In MINGW, malloc.h has explicit check for GNUC and if it is so, defines >> alloca to __builtin_alloca, so it looks like we don't need to add any >> code to here-shipped alloca.h to get optimum performance. >> >> Compile-tested on Windows in MSysGit. >> >> Signed-off-by: Kirill Smelkov > > Looks good to me! Thanks; queued and pushed out on 'next'. -- 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 17/19] Portable alloca for Git
On Wed, Apr 9, 2014 at 2:48 PM, Kirill Smelkov wrote: > On Thu, Mar 27, 2014 at 06:22:50PM +0400, Kirill Smelkov wrote: >> On Mon, Mar 24, 2014 at 02:47:24PM -0700, Junio C Hamano wrote: >> > Kirill Smelkov writes: >> > >> > > On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote: >> > >> On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov wrote: >> > >> ... >> > >> > In fact that would be maybe preferred, for maintainers to enable >> > >> > alloca >> > >> > with knowledge and testing, as one person can't have them all at hand. >> > >> >> > >> Yeah, you're probably right. >> > > >> > > Erik, the patch has been merged into pu today. Would you please >> > > follow-up with tested MINGW change? >> > >> > Sooo I lost track but this discussion seems to have petered out >> > around here. I think the copy we have had for a while on 'pu' is >> > basically sound, and can easily built on by platform folks by adding >> > or removing the -DHAVE_ALLOCA_H from the Makefile. >> >> Yes, that is all correct - that version works and we can improve it in >> the future with platform-specific follow-up patches, if needed. > > Junio, thanks for merging this and other diff-tree patches to next. It > so happened that I'm wrestling with MSysGit today, so please also find > alloca-for-mingw patch attached below. > > Thanks, > Kirill > > 8< > Subject: [PATCH] mingw: activate alloca > > Both MSVC and MINGW have alloca(3) definitions in malloc.h, so by moving > win32-compat alloca.h from compat/vcbuild/include/ to compat/win32/ , > which is included by both MSVC and MINGW CFLAGS, we can make alloca() > work on both those Windows environments. > > In MINGW, malloc.h has explicit check for GNUC and if it is so, defines > alloca to __builtin_alloca, so it looks like we don't need to add any > code to here-shipped alloca.h to get optimum performance. > > Compile-tested on Windows in MSysGit. > > Signed-off-by: Kirill Smelkov Looks good to me! -- 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 17/19] Portable alloca for Git
On Thu, Mar 27, 2014 at 06:22:50PM +0400, Kirill Smelkov wrote:
> On Mon, Mar 24, 2014 at 02:47:24PM -0700, Junio C Hamano wrote:
> > Kirill Smelkov writes:
> >
> > > On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote:
> > >> On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov wrote:
> > >> ...
> > >> > In fact that would be maybe preferred, for maintainers to enable alloca
> > >> > with knowledge and testing, as one person can't have them all at hand.
> > >>
> > >> Yeah, you're probably right.
> > >
> > > Erik, the patch has been merged into pu today. Would you please
> > > follow-up with tested MINGW change?
> >
> > Sooo I lost track but this discussion seems to have petered out
> > around here. I think the copy we have had for a while on 'pu' is
> > basically sound, and can easily built on by platform folks by adding
> > or removing the -DHAVE_ALLOCA_H from the Makefile.
>
> Yes, that is all correct - that version works and we can improve it in
> the future with platform-specific follow-up patches, if needed.
Junio, thanks for merging this and other diff-tree patches to next. It
so happened that I'm wrestling with MSysGit today, so please also find
alloca-for-mingw patch attached below.
Thanks,
Kirill
8<
Subject: [PATCH] mingw: activate alloca
Both MSVC and MINGW have alloca(3) definitions in malloc.h, so by moving
win32-compat alloca.h from compat/vcbuild/include/ to compat/win32/ ,
which is included by both MSVC and MINGW CFLAGS, we can make alloca()
work on both those Windows environments.
In MINGW, malloc.h has explicit check for GNUC and if it is so, defines
alloca to __builtin_alloca, so it looks like we don't need to add any
code to here-shipped alloca.h to get optimum performance.
Compile-tested on Windows in MSysGit.
Signed-off-by: Kirill Smelkov
---
compat/{vcbuild/include => win32}/alloca.h | 0
config.mak.uname | 1 +
2 files changed, 1 insertion(+)
rename compat/{vcbuild/include => win32}/alloca.h (100%)
diff --git a/compat/vcbuild/include/alloca.h b/compat/win32/alloca.h
similarity index 100%
rename from compat/vcbuild/include/alloca.h
rename to compat/win32/alloca.h
diff --git a/config.mak.uname b/config.mak.uname
index 17ef893..67bc054 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -480,6 +480,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
endif
ifneq (,$(findstring MINGW,$(uname_S)))
pathsep = ;
+ HAVE_ALLOCA_H = YesPlease
NO_PREAD = YesPlease
NEEDS_CRYPTO_WITH_SSL = YesPlease
NO_LIBGEN_H = YesPlease
--
1.9.0.msysgit.0.31.g74d1b9a.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 17/19] Portable alloca for Git
On Mon, Mar 24, 2014 at 02:47:24PM -0700, Junio C Hamano wrote:
> Kirill Smelkov writes:
>
> > On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote:
> >> On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov wrote:
> >> ...
> >> > In fact that would be maybe preferred, for maintainers to enable alloca
> >> > with knowledge and testing, as one person can't have them all at hand.
> >>
> >> Yeah, you're probably right.
> >
> > Erik, the patch has been merged into pu today. Would you please
> > follow-up with tested MINGW change?
>
> Sooo I lost track but this discussion seems to have petered out
> around here. I think the copy we have had for a while on 'pu' is
> basically sound, and can easily built on by platform folks by adding
> or removing the -DHAVE_ALLOCA_H from the Makefile.
Yes, that is all correct - that version works and we can improve it in
the future with platform-specific follow-up patches, if needed.
Please pick up the patch with ack from Thomas Schwinge.
Thanks,
Kirill
(please keep author email)
8<
From: Kirill Smelkov
Date: Mon, 24 Feb 2014 20:21:49 +0400
Subject: [PATCH v1a] Portable alloca for Git
In the next patch we'll have to use alloca() for performance reasons,
but since alloca is non-standardized and is not portable, let's have a
trick with compatibility wrappers:
1. at configure time, determine, do we have working alloca() through
alloca.h, and define
#define HAVE_ALLOCA_H
if yes.
2. in code
#ifdef HAVE_ALLOCA_H
# include
# define xalloca(size) (alloca(size))
# define xalloca_free(p)do {} while(0)
#else
# define xalloca(size) (xmalloc(size))
# define xalloca_free(p)(free(p))
#endif
and use it like
func() {
p = xalloca(size);
...
xalloca_free(p);
}
This way, for systems, where alloca is available, we'll have optimal
on-stack allocations with fast executions. On the other hand, on
systems, where alloca is not available, this gracefully fallbacks to
xmalloc/free.
Both autoconf and config.mak.uname configurations were updated. For
autoconf, we are not bothering considering cases, when no alloca.h is
available, but alloca() works some other way - its simply alloca.h is
available and works or not, everything else is deep legacy.
For config.mak.uname, I've tried to make my almost-sure guess for where
alloca() is available, but since I only have access to Linux it is the
only change I can be sure about myself, with relevant to other changed
systems people Cc'ed.
NOTE
SunOS and Windows had explicit -DHAVE_ALLOCA_H in their configurations.
I've changed that to now-common HAVE_ALLOCA_H=YesPlease which should be
correct.
Cc: Brandon Casey
Cc: Marius Storm-Olsen
Cc: Johannes Sixt
Cc: Johannes Schindelin
Cc: Ramsay Jones
Cc: Gerrit Pape
Cc: Petr Salinger
Cc: Jonathan Nieder
Cc: Thomas Schwinge
Acked-by: Thomas Schwinge (GNU Hurd changes)
Signed-off-by: Kirill Smelkov
Signed-off-by: Junio C Hamano
---
Changes since v1:
- added ack for GNU/Hurd.
Makefile | 6 ++
config.mak.uname | 10 --
configure.ac | 8
git-compat-util.h | 8
4 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index dddaf4f..0334806 100644
--- a/Makefile
+++ b/Makefile
@@ -30,6 +30,8 @@ all::
# Define LIBPCREDIR=/foo/bar if your libpcre header and library files are in
# /foo/bar/include and /foo/bar/lib directories.
#
+# Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
+#
# Define NO_CURL if you do not have libcurl installed. git-http-fetch and
# git-http-push are not built, and you cannot use http:// and https://
# transports (neither smart nor dumb).
@@ -1099,6 +1101,10 @@ ifdef USE_LIBPCRE
EXTLIBS += -lpcre
endif
+ifdef HAVE_ALLOCA_H
+ BASIC_CFLAGS += -DHAVE_ALLOCA_H
+endif
+
ifdef NO_CURL
BASIC_CFLAGS += -DNO_CURL
REMOTE_CURL_PRIMARY =
diff --git a/config.mak.uname b/config.mak.uname
index 7d31fad..71602ee 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -28,6 +28,7 @@ ifeq ($(uname_S),OSF1)
NO_NSEC = YesPlease
endif
ifeq ($(uname_S),Linux)
+ HAVE_ALLOCA_H = YesPlease
NO_STRLCPY = YesPlease
NO_MKSTEMPS = YesPlease
HAVE_PATHS_H = YesPlease
@@ -35,6 +36,7 @@ ifeq ($(uname_S),Linux)
HAVE_DEV_TTY = YesPlease
endif
ifeq ($(uname_S),GNU/kFreeBSD)
+ HAVE_ALLOCA_H = YesPlease
NO_STRLCPY = YesPlease
NO_MKSTEMPS = YesPlease
HAVE_PATHS_H = YesPlease
@@ -103,6 +105,7 @@ ifeq ($(uname_S),SunOS)
NEEDS_NSL = YesPlease
SHELL_PATH = /bin/bash
SANE_TOOL_PATH = /usr/xpg6/bin:/usr/xpg4/bin
+ HAVE_ALLOCA_H = YesPlease
NO_STRCASESTR = YesPlease
NO_MEMMEM = YesPlease
NO_MKDTEMP = YesPlease
@@ -146,7 +149,7 @@ ifeq ($(uname_S),SunOS)
endif
INSTALL = /usr/ucb/install
TAR = gtar
- BASIC_CFLA
Re: [PATCH 17/19] Portable alloca for Git
Kirill Smelkov writes: > On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote: >> On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov wrote: >> ... >> > In fact that would be maybe preferred, for maintainers to enable alloca >> > with knowledge and testing, as one person can't have them all at hand. >> >> Yeah, you're probably right. > > Erik, the patch has been merged into pu today. Would you please > follow-up with tested MINGW change? Sooo I lost track but this discussion seems to have petered out around here. I think the copy we have had for a while on 'pu' is basically sound, and can easily built on by platform folks by adding or removing the -DHAVE_ALLOCA_H from the Makefile. -- 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 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote: > On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov wrote: > > On Fri, Feb 28, 2014 at 02:50:04PM +0100, Erik Faye-Lund wrote: > >> On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund > >> wrote: > >> > On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov wrote: > >> >> diff --git a/Makefile b/Makefile > >> >> index dddaf4f..0334806 100644 > >> >> --- a/Makefile > >> >> +++ b/Makefile > >> >> @@ -316,6 +321,7 @@ endif > >> >> ifeq ($(uname_S),Windows) > >> >> GIT_VERSION := $(GIT_VERSION).MSVC > >> >> pathsep = ; > >> >> + HAVE_ALLOCA_H = YesPlease > >> >> NO_PREAD = YesPlease > >> >> NEEDS_CRYPTO_WITH_SSL = YesPlease > >> >> NO_LIBGEN_H = YesPlease > >> > > >> > In MSVC, alloca is defined in in malloc.h, not alloca.h: > >> > > >> > http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx > >> > > >> > In fact, it has no alloca.h at all. But we don't have malloca.h in > >> > mingw either, so creating a compat/win32/alloca.h that includes > >> > malloc.h is probably sufficient. > >> > >> "But we don't have alloca.h in mingw either", sorry. > > > > Don't we have that for MSVC already in > > > > compat/vcbuild/include/alloca.h > > > > and > > > > ifeq ($(uname_S),Windows) > > ... > > BASIC_CFLAGS = ... -Icompat/vcbuild/include ... > > > > > > in config.mak.uname ? > > Ah, of course. Thanks for setting me straight! > > > And as I've not touched MINGW part in config.mak.uname the patch stays > > valid as it is :) and we can incrementally update what platforms have > > working alloca with follow-up patches. > > > > In fact that would be maybe preferred, for maintainers to enable alloca > > with knowledge and testing, as one person can't have them all at hand. > > Yeah, you're probably right. Erik, the patch has been merged into pu today. Would you please follow-up with tested MINGW change? Thanks beforehand, Kirill -- 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 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov wrote: > On Fri, Feb 28, 2014 at 02:50:04PM +0100, Erik Faye-Lund wrote: >> On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund wrote: >> > On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov wrote: >> >> diff --git a/Makefile b/Makefile >> >> index dddaf4f..0334806 100644 >> >> --- a/Makefile >> >> +++ b/Makefile >> >> @@ -316,6 +321,7 @@ endif >> >> ifeq ($(uname_S),Windows) >> >> GIT_VERSION := $(GIT_VERSION).MSVC >> >> pathsep = ; >> >> + HAVE_ALLOCA_H = YesPlease >> >> NO_PREAD = YesPlease >> >> NEEDS_CRYPTO_WITH_SSL = YesPlease >> >> NO_LIBGEN_H = YesPlease >> > >> > In MSVC, alloca is defined in in malloc.h, not alloca.h: >> > >> > http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx >> > >> > In fact, it has no alloca.h at all. But we don't have malloca.h in >> > mingw either, so creating a compat/win32/alloca.h that includes >> > malloc.h is probably sufficient. >> >> "But we don't have alloca.h in mingw either", sorry. > > Don't we have that for MSVC already in > > compat/vcbuild/include/alloca.h > > and > > ifeq ($(uname_S),Windows) > ... > BASIC_CFLAGS = ... -Icompat/vcbuild/include ... > > > in config.mak.uname ? Ah, of course. Thanks for setting me straight! > And as I've not touched MINGW part in config.mak.uname the patch stays > valid as it is :) and we can incrementally update what platforms have > working alloca with follow-up patches. > > In fact that would be maybe preferred, for maintainers to enable alloca > with knowledge and testing, as one person can't have them all at hand. Yeah, you're probably right. -- 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 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 02:50:04PM +0100, Erik Faye-Lund wrote: > On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund wrote: > > On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov wrote: > >> diff --git a/Makefile b/Makefile > >> index dddaf4f..0334806 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -316,6 +321,7 @@ endif > >> ifeq ($(uname_S),Windows) > >> GIT_VERSION := $(GIT_VERSION).MSVC > >> pathsep = ; > >> + HAVE_ALLOCA_H = YesPlease > >> NO_PREAD = YesPlease > >> NEEDS_CRYPTO_WITH_SSL = YesPlease > >> NO_LIBGEN_H = YesPlease > > > > In MSVC, alloca is defined in in malloc.h, not alloca.h: > > > > http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx > > > > In fact, it has no alloca.h at all. But we don't have malloca.h in > > mingw either, so creating a compat/win32/alloca.h that includes > > malloc.h is probably sufficient. > > "But we don't have alloca.h in mingw either", sorry. Don't we have that for MSVC already in compat/vcbuild/include/alloca.h and ifeq ($(uname_S),Windows) ... BASIC_CFLAGS = ... -Icompat/vcbuild/include ... in config.mak.uname ? And as I've not touched MINGW part in config.mak.uname the patch stays valid as it is :) and we can incrementally update what platforms have working alloca with follow-up patches. In fact that would be maybe preferred, for maintainers to enable alloca with knowledge and testing, as one person can't have them all at hand. Thanks, Kirill -- 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 17/19] Portable alloca for Git
On Fri, Feb 28, 2014 at 2:44 PM, Erik Faye-Lund wrote: > On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov wrote: >> diff --git a/Makefile b/Makefile >> index dddaf4f..0334806 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -316,6 +321,7 @@ endif >> ifeq ($(uname_S),Windows) >> GIT_VERSION := $(GIT_VERSION).MSVC >> pathsep = ; >> + HAVE_ALLOCA_H = YesPlease >> NO_PREAD = YesPlease >> NEEDS_CRYPTO_WITH_SSL = YesPlease >> NO_LIBGEN_H = YesPlease > > In MSVC, alloca is defined in in malloc.h, not alloca.h: > > http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx > > In fact, it has no alloca.h at all. But we don't have malloca.h in > mingw either, so creating a compat/win32/alloca.h that includes > malloc.h is probably sufficient. "But we don't have alloca.h in mingw either", sorry. -- 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 17/19] Portable alloca for Git
On Mon, Feb 24, 2014 at 5:21 PM, Kirill Smelkov wrote: > diff --git a/Makefile b/Makefile > index dddaf4f..0334806 100644 > --- a/Makefile > +++ b/Makefile > @@ -316,6 +321,7 @@ endif > ifeq ($(uname_S),Windows) > GIT_VERSION := $(GIT_VERSION).MSVC > pathsep = ; > + HAVE_ALLOCA_H = YesPlease > NO_PREAD = YesPlease > NEEDS_CRYPTO_WITH_SSL = YesPlease > NO_LIBGEN_H = YesPlease In MSVC, alloca is defined in in malloc.h, not alloca.h: http://msdn.microsoft.com/en-us/library/wb1s57t5.aspx In fact, it has no alloca.h at all. But we don't have malloca.h in mingw either, so creating a compat/win32/alloca.h that includes malloc.h is probably sufficient. -- 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 17/19] Portable alloca for Git
Hi! On Mon, 24 Feb 2014 20:21:49 +0400, Kirill Smelkov wrote: > Both autoconf and config.mak.uname configurations were updated. For > autoconf, we are not bothering considering cases, when no alloca.h is > available, but alloca() works some other way - its simply alloca.h is > available and works or not, everything else is deep legacy. Sounds good for GNU Hurd, or any system using glibc (but have not explicitly tested your patch). > For config.mak.uname, I've tried to make my almost-sure guess for where > alloca() is available, but since I only have access to Linux it is the > only change I can be sure about myself, with relevant to other changed > systems people Cc'ed. > diff --git a/config.mak.uname b/config.mak.uname > index 7d31fad..71602ee 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -239,6 +243,7 @@ ifeq ($(uname_S),AIX) > endif > ifeq ($(uname_S),GNU) > # GNU/Hurd > + HAVE_ALLOCA_H = YesPlease > NO_STRLCPY = YesPlease > NO_MKSTEMPS = YesPlease > HAVE_PATHS_H = YesPlease Acked-by: Thomas Schwinge (GNU Hurd changes) Grüße, Thomas pgphu749DPUFO.pgp Description: PGP signature
