Re: [RFC/PATCH] Update compat/regex

Ævar Arnfjörð Bjarmason [off-list ref] writes:

This patch has all the glibc-specific stuff that makes it break hard
if you don't have the GNU C library. Writing macros/definitions to fix all that
stuff up was the "easier said than done" part I was referring to.

You might want to try out the gnulib version instead.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

On Fri, Jul 16, 2010 at 14:17, Andreas Schwab [off-list ref] wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

This patch has all the glibc-specific stuff that makes it break hard
if you don't have the GNU C library. Writing macros/definitions to fix all that
stuff up was the "easier said than done" part I was referring to.

You might want to try out the gnulib version instead.

I fiddled a bit with gnulib for both the regex engine and libintl, but
I can't get it to do what I want.

The assumption with gnulib seems to be that you're including the
libraries in a GNU program that only uses the autotools, it seems to
be about as easy to just copy/paste things from glibc if you're adding
libraries to a program like Git that uses its own build system.

On 08/15/2010 01:08 PM, Ævar Arnfjörð Bjarmason wrote:

On Fri, Jul 16, 2010 at 14:17, Andreas Schwab[off-list ref]  wrote:

quoted

Ævar Arnfjörð Bjarmason[off-list ref]  writes:

quoted

This patch has all the glibc-specific stuff that makes it break hard
if you don't have the GNU C library. Writing macros/definitions to fix all that
stuff up was the "easier said than done" part I was referring to.

You might want to try out the gnulib version instead.

I fiddled a bit with gnulib for both the regex engine and libintl, but
I can't get it to do what I want.

The assumption with gnulib seems to be that you're including the
libraries in a GNU program that only uses the autotools, it seems to
be about as easy to just copy/paste things from glibc if you're adding
libraries to a program like Git that uses its own build system.

Andreas is right, the glibc code is not meant to be portable.

It is really simpler if you start from the version in gnulib, which is 
the one that is included in most GNU packages nowadays.  You should 
download GNU grep 2.6.x and (starting from lib/reg*) add headers from 
its lib/ directory until it compiles.

Alternatively try out gawk, as it does not use gnulib but has the same 
set of sanitizations.

Paolo

On Mon, Aug 16, 2010 at 12:26, Paolo Bonzini [off-list ref] wrote:

Alternatively try out gawk, as it does not use gnulib but has the same set
of sanitizations.

Why didn't you say that earlier? :)

Here's a RFC patch series that uses the gawk regex engine from the
gawk-devel branch of gawk CVS.

It compiles on Linux/FreeBSD and Solaris, with only a single warning
on FreeBSD due to an unused variable (upstream bug).

Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: hacks to get the gawk regex engine to compile within
    git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/COPYING          |  674 ++++++
 compat/regex/mbsupport.h      |   59 +
 compat/regex/regcomp.c        | 3892 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 t/t7008-grep-binary.sh        |    2 +-
 10 files changed, 11921 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/COPYING
 create mode 100644 compat/regex/mbsupport.h
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

-- 
1.7.2.1.389.gc3d0b

The gawk regex engine didn't include stdio.h, and only include
stddef.h if HAVE_STDDEF_H is set.

Adding -DHAVE_STDDEF_H caused some internal errors in by /usr/include
headers, so change the regex.h code to include it unconditionally.

We also need to define -DGAWK so that e.g. "bool", "MAX" and other
similar things used inside gawk get defined.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Makefile             |    4 ++++
 compat/regex/regex.h |    7 +++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index b4745a5..6704780 100644
--- a/Makefile
+++ b/Makefile

@@ -1443,6 +1443,10 @@ ifdef UNRELIABLE_FSTAT
 	BASIC_CFLAGS += -DUNRELIABLE_FSTAT
 endif
 ifdef NO_REGEX
+	# TODO: How do I compile just regex.o with this flag, not the
+	# whole of Git?
+	BASIC_CFLAGS += -DGAWK
+
 	COMPAT_CFLAGS += -Icompat/regex
 	COMPAT_OBJS += compat/regex/regex.o
 endif

diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index de93327..508bc80 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h

@@ -22,9 +22,12 @@
 #ifndef _REGEX_H
 #define _REGEX_H 1
 
-#ifdef HAVE_STDDEF_H
+#include <stdio.h>
+/*
+  Git: Was in `#ifdef HAVE_STDDEF_H` in gawk, adding -DHAVE_STDDEF_H makes a
+  *lot* of other things break
+*/
 #include <stddef.h>
-#endif
 
 #ifdef HAVE_SYS_TYPES_H
 #include <sys/types.h>

-- 
1.7.2.1.389.gc3d0b

Now that we have a regex engine that supports REG_STARTEND this test
should fail if "git grep" can't grep NULL characters. Platforms that
don't have a POSIX regex engine that supports REG_STARTEND should
always define NO_REGEX=YesPlease when compiling.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t7008-grep-binary.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..c0f9f3f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh

@@ -61,7 +61,7 @@ test_expect_success 'git grep -Fi iLE a' '
 
 # This test actually passes on platforms where regexec() supports the
 # flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
+test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 

-- 
1.7.2.1.389.gc3d0b

Hi!

Ævar Arnfjörð Bjarmason wrote:

+	# TODO: How do I compile just regex.o with this flag, not the
+	# whole of Git?
+	BASIC_CFLAGS += -DGAWK

 gawk.o: EXTRA_CPPFLAGS = -DGAWK

See v1.7.1-rc0~60 and v1.7.0-rc0~90^2~2.

Maybe

 #if 1
 #include "git-compat-util.h"
 #else
 ...

would be simpler.

This has the following changes:

 * Supply the custom regex.o flags only to regex.o as suggested by
   Jonathan Nieder:
        
        +ifdef NO_REGEX
        +compat/regex/regex.o: EXTRA_CPPFLAGS = -DGAWK -DNO_MBSUPPORT
        +endif

 * The code is LGPL-2.1, not GPL-3

 * Don't include mbsupport.h, we don't need it, and it can be
   un-included with a flag.

 * Simplify our modifications to regex.h, just include two headers at
   the very top, don't modify any gawk code.

 * Update commit messages
        
Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: get the gawk regex engine to compile within git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/regcomp.c        | 3892 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 t/t7008-grep-binary.sh        |    2 +-
 8 files changed, 11188 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

-- 
1.7.2.1.389.gc3d0b

We need to define -DGAWK -DNO_MBSUPPORT so that the gawk regex engine
will compile, and include stdio.h and stddef.h in regex.h. Gawk itself
includes these headers before it includes the regex.h header.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Makefile             |    4 ++++
 compat/regex/regex.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index b4745a5..23a9f0d 100644
--- a/Makefile
+++ b/Makefile

@@ -1879,6 +1879,10 @@ ifdef NO_EXPAT
 http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
 endif
 
+ifdef NO_REGEX
+compat/regex/regex.o: EXTRA_CPPFLAGS = -DGAWK -DNO_MBSUPPORT
+endif
+
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 

diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index de93327..61c9683 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h

@@ -1,3 +1,6 @@
+#include <stdio.h>
+#include <stddef.h>
+
 /* Definitions for data structures and routines for the regular
    expression library.
    Copyright (C) 1985,1989-93,1995-98,2000,2001,2002,2003,2005,2006,2008

-- 
1.7.2.1.389.gc3d0b

Now that we have a regex engine that supports REG_STARTEND this test
should fail if "git grep" can't grep NULL characters.

Platforms that don't have a POSIX regex engine which supports
REG_STARTEND should always define NO_REGEX=YesPlease when compiling.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t7008-grep-binary.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..c0f9f3f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh

@@ -61,7 +61,7 @@ test_expect_success 'git grep -Fi iLE a' '
 
 # This test actually passes on platforms where regexec() supports the
 # flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
+test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 

-- 
1.7.2.1.389.gc3d0b

Ævar Arnfjörð Bjarmason wrote:

Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: get the gawk regex engine to compile within git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

For what it’s worth:

Acked-by: Jonathan Nieder <redacted>

Thanks.  Here’s a patch to go on top.  I am not sure what platforms
support REG_STARTEND, but we can always update the makefile as reports
come in.

-- 8< --
Subject: autoconf: don't use platform regex if it lacks REG_STARTEND

If the platform regex cannot match null bytes, we might as well
use the glibc version instead.

Cc: Ævar Arnfjörð Bjarmason <redacted>
Cc: René Scharfe <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
 config.mak.in |    1 +
 configure.ac  |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index b4e65c3..67dbd3b 100644
--- a/config.mak.in
+++ b/config.mak.in

@@ -58,6 +58,7 @@ NO_INET_NTOP=@NO_INET_NTOP@
 NO_INET_PTON=@NO_INET_PTON@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
+NO_REGEX=@NO_REGEX@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
 INLINE=@INLINE@
 SOCKLEN_T=@SOCKLEN_T@

diff --git a/configure.ac b/configure.ac
index 5601e8b..71ac89f 100644
--- a/configure.ac
+++ b/configure.ac

@@ -706,6 +706,27 @@ else
 fi
 AC_SUBST(NO_C99_FORMAT)
 #
+# Define NO_REGEX if you have no or inferior regex support in your C library.
+AC_CACHE_CHECK([whether the platform regex can handle null bytes],
+ [ac_cv_c_excellent_regex], [
+AC_EGREP_CPP(yippeeyeswehaveit,
+	AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+#include <regex.h>
+],
+[#ifdef REG_STARTEND
+yippeeyeswehaveit
+#endif
+]),
+	[ac_cv_c_excellent_regex=yes],
+	[ac_cv_c_excellent_regex=yes])
+])
+if test $ac_cv_c_excellent_regex = yes; then
+	NO_REGEX=
+else
+	NO_REGEX=YesPlease
+fi
+AC_SUBST(NO_REGEX)
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],

-- 
1.7.2.1.544.ga752d.dirty

Here's a (hopefully) final version of this series. Changes since the
v2 RFC:

 * Re-apply Frank Li's regerror() patch already in Git as
   v1.6.5-rc2~23. There was no need to apply another msvc fix,
   v1.7.0-rc0~15, because it had already been fixed upstream.

 * Include Jonathan Nieder's autoconf patch and add his Acked-by to
   the rest, add my Tested-by to his.

 * Fix text alignment in one of the commit messages.

Note: This patch is intentionally diff --check unclean so we don't
diverge from upstream. This series can also be pulled from
http://github.com/avar/git/tree/update-fallback-regex-engine-v3 if the
whitespace causes issues with git-am.

Frank Li (1):
  Change regerror() declaration from K&R style to ANSI C (C89)

Jonathan Nieder (1):
  autoconf: don't use platform regex if it lacks REG_STARTEND

Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: get the gawk regex engine to compile within git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/regcomp.c        | 3889 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 config.mak.in                 |    1 +
 configure.ac                  |   21 +
 t/t7008-grep-binary.sh        |    2 +-
 10 files changed, 11207 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

-- 
1.7.2.1.389.gc3d0b

Now that we have a regex engine that supports REG_STARTEND this test
should fail if "git grep" can't grep NULL characters.

Platforms that don't have a POSIX regex engine which supports
REG_STARTEND should always define NO_REGEX=YesPlease when compiling.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Acked-by: Jonathan Nieder <redacted>
---
 t/t7008-grep-binary.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..c0f9f3f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh

@@ -61,7 +61,7 @@ test_expect_success 'git grep -Fi iLE a' '
 
 # This test actually passes on platforms where regexec() supports the
 # flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
+test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 

-- 
1.7.2.1.389.gc3d0b

From: Frank Li <redacted>

The MSVC headers typedef errcode as int, and thus confused the compiler in
the K&R style definition. ANSI style deconfuses it.

This patch was originally applied as v1.6.5-rc2~23 but needs to be
re-applied since compat/regex was overwritten by Ævar Arnfjörð
Bjarmason with the gawk regex engine.

Signed-off-by: Frank Li <redacted>
Signed-off-by: Marius Storm-Olsen <redacted>
Acked-by: Johannes Sixt <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 compat/regex/regcomp.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 5115d7a..647c22a 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c

@@ -546,11 +546,8 @@ weak_alias (__regcomp, regcomp)
    from either regcomp or regexec.   We don't use PREG here.  */
 
 size_t
-regerror (errcode, preg, errbuf, errbuf_size)
-    int errcode;
-    const regex_t *__restrict preg;
-    char *__restrict errbuf;
-    size_t errbuf_size;
+regerror(int errcode, const regex_t *__restrict preg,
+         char *__restrict errbuf, size_t errbuf_size)
 {
   const char *msg;
   size_t msg_size;

-- 
1.7.2.1.389.gc3d0b

We need to define -DGAWK -DNO_MBSUPPORT so that the gawk regex engine
will compile, and include stdio.h and stddef.h in regex.h. Gawk itself
includes these headers before it includes the regex.h header.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Acked-by: Jonathan Nieder <redacted>
---
 Makefile             |    4 ++++
 compat/regex/regex.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index b4745a5..23a9f0d 100644
--- a/Makefile
+++ b/Makefile

@@ -1879,6 +1879,10 @@ ifdef NO_EXPAT
 http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
 endif
 
+ifdef NO_REGEX
+compat/regex/regex.o: EXTRA_CPPFLAGS = -DGAWK -DNO_MBSUPPORT
+endif
+
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 

diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index de93327..61c9683 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h

@@ -1,3 +1,6 @@
+#include <stdio.h>
+#include <stddef.h>
+
 /* Definitions for data structures and routines for the regular
    expression library.
    Copyright (C) 1985,1989-93,1995-98,2000,2001,2002,2003,2005,2006,2008

-- 
1.7.2.1.389.gc3d0b

From: Jonathan Nieder <redacted>

If the platform regex cannot match null bytes, we might as well
use the glibc version instead.

Cc: Ævar Arnfjörð Bjarmason <redacted>
Cc: René Scharfe <redacted>
Signed-off-by: Jonathan Nieder <redacted>
Tested-by: Ævar Arnfjörð Bjarmason <redacted>
---
 config.mak.in |    1 +
 configure.ac  |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index b4e65c3..67dbd3b 100644
--- a/config.mak.in
+++ b/config.mak.in

@@ -58,6 +58,7 @@ NO_INET_NTOP=@NO_INET_NTOP@
 NO_INET_PTON=@NO_INET_PTON@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
+NO_REGEX=@NO_REGEX@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
 INLINE=@INLINE@
 SOCKLEN_T=@SOCKLEN_T@

diff --git a/configure.ac b/configure.ac
index 5601e8b..71ac89f 100644
--- a/configure.ac
+++ b/configure.ac

@@ -706,6 +706,27 @@ else
 fi
 AC_SUBST(NO_C99_FORMAT)
 #
+# Define NO_REGEX if you have no or inferior regex support in your C library.
+AC_CACHE_CHECK([whether the platform regex can handle null bytes],
+ [ac_cv_c_excellent_regex], [
+AC_EGREP_CPP(yippeeyeswehaveit,
+	AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+#include <regex.h>
+],
+[#ifdef REG_STARTEND
+yippeeyeswehaveit
+#endif
+]),
+	[ac_cv_c_excellent_regex=yes],
+	[ac_cv_c_excellent_regex=yes])
+])
+if test $ac_cv_c_excellent_regex = yes; then
+	NO_REGEX=
+else
+	NO_REGEX=YesPlease
+fi
+AC_SUBST(NO_REGEX)
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],

-- 
1.7.2.1.389.gc3d0b

On 08/17/2010 11:24 AM, Ævar Arnfjörð Bjarmason wrote:

   compat/regex: use the regex engine from gawk for compat
   compat/regex: get the gawk regex engine to compile within git

Should these two be squashed to ensure bisectability over a wide range 
of host systems?

Paolo