[PATCH] compat/regex: fix compilation on Windows

[PATCH] compat/regex: fix compilation on Windows

[Johannes Schindelin]

The real issue here is that GNU awk's regex implementation assumes a bit
too much about the relative sizes of pointers and long integers. What they
really want is to use intptr_t.

This patch recapitulates what 56a1a3ab449 (Silence GCC's "cast of pointer
to integer of a different size" warning, 2015-10-26) did to our previous
copy of GNU awk's regex engine.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/compat-regex-fixes-v1
Fetch-It-Via: git fetch https://github.com/dscho/git compat-regex-fixes-v1

 .../0003-Use-intptr_t-instead-of-long.patch        | 22 ++++++++++++++++++++++
 compat/regex/regcomp.c                             |  4 ++--
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch

diff --git a/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch b/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
new file mode 100644
index 00000000000..246ff256fb8
--- /dev/null
+++ b/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
@@ -0,0 +1,22 @@
+diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
+index 5e9ea26cd46..e6469167a80 100644
+--- a/compat/regex/regcomp.c
++++ b/compat/regex/regcomp.c
+@@ -2641,7 +2641,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
+     old_tree = NULL;
+ 
+   if (elem->token.type == SUBEXP)
+-    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
++    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
+ 
+   tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
+   if (BE (tree == NULL, 0))
+@@ -3868,7 +3868,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
+ static reg_errcode_t
+ mark_opt_subexp (void *extra, bin_tree_t *node)
+ {
+-  int idx = (int) (long) extra;
++  int idx = (int) (intptr_t) extra;
+   if (node->token.type == SUBEXP && node->token.opr.idx == idx)
+     node->token.opt_subexp = 1;
+ 
diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 5e9ea26cd46..e6469167a80 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -2641,7 +2641,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
     old_tree = NULL;
 
   if (elem->token.type == SUBEXP)
-    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
+    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
 
   tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
   if (BE (tree == NULL, 0))
@@ -3868,7 +3868,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
 static reg_errcode_t
 mark_opt_subexp (void *extra, bin_tree_t *node)
 {
-  int idx = (int) (long) extra;
+  int idx = (int) (intptr_t) extra;
   if (node->token.type == SUBEXP && node->token.opr.idx == idx)
     node->token.opt_subexp = 1;
 

base-commit: 4e23cefb4da69a2d884c2d5a303825f40008ca42
-- 
2.12.2.windows.2.800.gede8f145e06

Re: [PATCH] compat/regex: fix compilation on Windows

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 3:50 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> The real issue here is that GNU awk's regex implementation assumes a bit
> too much about the relative sizes of pointers and long integers. What they
> really want is to use intptr_t.

Thanks, looks good!

> This patch recapitulates what 56a1a3ab449 (Silence GCC's "cast of pointer
> to integer of a different size" warning, 2015-10-26) did to our previous
> copy of GNU awk's regex engine.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/compat-regex-fixes-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git compat-regex-fixes-v1
>
>  .../0003-Use-intptr_t-instead-of-long.patch        | 22 ++++++++++++++++++++++
>  compat/regex/regcomp.c                             |  4 ++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
>
> diff --git a/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch b/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
> new file mode 100644
> index 00000000000..246ff256fb8
> --- /dev/null
> +++ b/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
> @@ -0,0 +1,22 @@
> +diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
> +index 5e9ea26cd46..e6469167a80 100644
> +--- a/compat/regex/regcomp.c
> ++++ b/compat/regex/regcomp.c
> +@@ -2641,7 +2641,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
> +     old_tree = NULL;
> +
> +   if (elem->token.type == SUBEXP)
> +-    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
> ++    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
> +
> +   tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
> +   if (BE (tree == NULL, 0))
> +@@ -3868,7 +3868,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
> + static reg_errcode_t
> + mark_opt_subexp (void *extra, bin_tree_t *node)
> + {
> +-  int idx = (int) (long) extra;
> ++  int idx = (int) (intptr_t) extra;
> +   if (node->token.type == SUBEXP && node->token.opr.idx == idx)
> +     node->token.opt_subexp = 1;
> +
> diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
> index 5e9ea26cd46..e6469167a80 100644
> --- a/compat/regex/regcomp.c
> +++ b/compat/regex/regcomp.c
> @@ -2641,7 +2641,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
>      old_tree = NULL;
>
>    if (elem->token.type == SUBEXP)
> -    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
> +    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
>
>    tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
>    if (BE (tree == NULL, 0))
> @@ -3868,7 +3868,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
>  static reg_errcode_t
>  mark_opt_subexp (void *extra, bin_tree_t *node)
>  {
> -  int idx = (int) (long) extra;
> +  int idx = (int) (intptr_t) extra;
>    if (node->token.type == SUBEXP && node->token.opr.idx == idx)
>      node->token.opt_subexp = 1;
>
>
> base-commit: 4e23cefb4da69a2d884c2d5a303825f40008ca42
> --
> 2.12.2.windows.2.800.gede8f145e06

Re: [PATCH] compat/regex: fix compilation on Windows

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The real issue here is that GNU awk's regex implementation assumes a bit
> too much about the relative sizes of pointers and long integers. What they
> really want is to use intptr_t.

Good.  I got annoyed enough to do the same myself before opening my
mailbox.  One thing that is curious about your version is that it
still has "#include <stdint.h>" behind HAVE_STDINT_H; when I tried
to compile an equivalent of your change last night, the compilation
failed because intptr_t wasn't available without exposing <stdint.h>

Where is Windows build getting its intptr_t, I wonder.

Re: [PATCH] compat/regex: fix compilation on Windows

[Johannes Schindelin]

Hi Junio,

On Fri, 12 May 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > The real issue here is that GNU awk's regex implementation assumes a
> > bit too much about the relative sizes of pointers and long integers.
> > What they really want is to use intptr_t.
> 
> Good.  I got annoyed enough to do the same myself before opening my
> mailbox.  One thing that is curious about your version is that it still
> has "#include <stdint.h>" behind HAVE_STDINT_H; when I tried to compile
> an equivalent of your change last night, the compilation failed because
> intptr_t wasn't available without exposing <stdint.h>
> 
> Where is Windows build getting its intptr_t, I wonder.

I'd place a bet on this part of compat/mingw.h for GCC builds:

	#ifdef __MINGW64_VERSION_MAJOR
	#include <stdint.h>
	#include <wchar.h>
	typedef _sigset_t sigset_t;
	#endif

and on this part of git-compat-util.h for MSVC builds:

	#ifndef NO_INTTYPES_H
	#include <inttypes.h>
	#else
	#include <stdint.h>
	#endif

For the record, it seems that our current version of compat/regex/regex.c
has this:

	/* On some systems, limits.h sets RE_DUP_MAX to a lower value than
	   GNU regex allows.  Include it before <regex.h>, which correctly
	   #undefs RE_DUP_MAX and sets it to the right value.  */
	#include <limits.h>
	#include <stdint.h>

while the one in `pu` lacks the last line. That may be the reason why
things compiled neatly before, and stopped working for you now.

So yeah, I think you are right in that the HAVE_STDINT_H guard needs to be
removed either before my patch, or squashed into it.

Ciao,
Dscho

Re: [PATCH] compat/regex: fix compilation on Windows

[Ramsay Jones]

On 12/05/17 11:25, Johannes Schindelin wrote:
> Hi Junio,
> 
> On Fri, 12 May 2017, Junio C Hamano wrote:
> 
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> The real issue here is that GNU awk's regex implementation assumes a
>>> bit too much about the relative sizes of pointers and long integers.
>>> What they really want is to use intptr_t.
>>
>> Good.  I got annoyed enough to do the same myself before opening my
>> mailbox.  One thing that is curious about your version is that it still
>> has "#include <stdint.h>" behind HAVE_STDINT_H; when I tried to compile
>> an equivalent of your change last night, the compilation failed because
>> intptr_t wasn't available without exposing <stdint.h>
>>
>> Where is Windows build getting its intptr_t, I wonder.
> 
> I'd place a bet on this part of compat/mingw.h for GCC builds:
> 
> 	#ifdef __MINGW64_VERSION_MAJOR
> 	#include <stdint.h>
> 	#include <wchar.h>
> 	typedef _sigset_t sigset_t;
> 	#endif
> 
> and on this part of git-compat-util.h for MSVC builds:
> 
> 	#ifndef NO_INTTYPES_H
> 	#include <inttypes.h>
> 	#else
> 	#include <stdint.h>
> 	#endif
> 
> For the record, it seems that our current version of compat/regex/regex.c
> has this:
> 
> 	/* On some systems, limits.h sets RE_DUP_MAX to a lower value than
> 	   GNU regex allows.  Include it before <regex.h>, which correctly
> 	   #undefs RE_DUP_MAX and sets it to the right value.  */
> 	#include <limits.h>
> 	#include <stdint.h>
> 
> while the one in `pu` lacks the last line. That may be the reason why
> things compiled neatly before, and stopped working for you now.

Yep, see commit bd8f005583. ;-P

So, sparse is, once again, complaining about the SIZE_MAX macro
redefinition. (Along with two other warnings, one of which is a
_very_ long standing warning and one of which is new - I have
yet to investigate).

ATB,
Ramsay Jones

Re: [PATCH] compat/regex: fix compilation on Windows

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I'd place a bet on this part of compat/mingw.h for GCC builds:
>
> 	#ifdef __MINGW64_VERSION_MAJOR
> 	#include <stdint.h>
> 	#include <wchar.h>
> 	typedef _sigset_t sigset_t;
> 	#endif
>
> and on this part of git-compat-util.h for MSVC builds:
>
> 	#ifndef NO_INTTYPES_H
> 	#include <inttypes.h>
> 	#else
> 	#include <stdint.h>
> 	#endif

Yeah, but I had an impression that git-compat-util.h was not in use
to build compat/regex/regex.o (regex.c #include's regcomp.c which is
a rather unusual arragement), and that was why I am wondering.

> For the record, it seems that our current version of compat/regex/regex.c
> has this:
>
> 	/* On some systems, limits.h sets RE_DUP_MAX to a lower value than
> 	   GNU regex allows.  Include it before <regex.h>, which correctly
> 	   #undefs RE_DUP_MAX and sets it to the right value.  */
> 	#include <limits.h>
> 	#include <stdint.h>
>
> while the one in `pu` lacks the last line. That may be the reason why
> things compiled neatly before, and stopped working for you now.

Yes, that explains how the old one worked and the new one didn't
without dropping "#ifdef HAVE_STDINT_H".

Thanks.

Re: [PATCH] compat/regex: fix compilation on Windows

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 3:50 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> The real issue here is that GNU awk's regex implementation assumes a bit
> too much about the relative sizes of pointers and long integers. What they
> really want is to use intptr_t.
>
> This patch recapitulates what 56a1a3ab449 (Silence GCC's "cast of pointer
> to integer of a different size" warning, 2015-10-26) did to our previous
> copy of GNU awk's regex engine.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/compat-regex-fixes-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git compat-regex-fixes-v1
>
>  .../0003-Use-intptr_t-instead-of-long.patch        | 22 ++++++++++++++++++++++
>  compat/regex/regcomp.c                             |  4 ++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
>
> diff --git a/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch b/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
> new file mode 100644
> index 00000000000..246ff256fb8
> --- /dev/null
> +++ b/compat/regex/patches/0003-Use-intptr_t-instead-of-long.patch
> @@ -0,0 +1,22 @@
> +diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
> +index 5e9ea26cd46..e6469167a80 100644
> +--- a/compat/regex/regcomp.c
> ++++ b/compat/regex/regcomp.c
> +@@ -2641,7 +2641,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
> +     old_tree = NULL;
> +
> +   if (elem->token.type == SUBEXP)
> +-    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
> ++    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
> +
> +   tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
> +   if (BE (tree == NULL, 0))
> +@@ -3868,7 +3868,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
> + static reg_errcode_t
> + mark_opt_subexp (void *extra, bin_tree_t *node)
> + {
> +-  int idx = (int) (long) extra;
> ++  int idx = (int) (intptr_t) extra;
> +   if (node->token.type == SUBEXP && node->token.opr.idx == idx)
> +     node->token.opt_subexp = 1;
> +
> diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
> index 5e9ea26cd46..e6469167a80 100644
> --- a/compat/regex/regcomp.c
> +++ b/compat/regex/regcomp.c
> @@ -2641,7 +2641,7 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, re_dfa_t *dfa,
>      old_tree = NULL;
>
>    if (elem->token.type == SUBEXP)
> -    postorder (elem, mark_opt_subexp, (void *) (long) elem->token.opr.idx);
> +    postorder (elem, mark_opt_subexp, (void *) (intptr_t) elem->token.opr.idx);
>
>    tree = create_tree (dfa, elem, NULL, (end == -1 ? OP_DUP_ASTERISK : OP_ALT));
>    if (BE (tree == NULL, 0))
> @@ -3868,7 +3868,7 @@ create_token_tree (re_dfa_t *dfa, bin_tree_t *left, bin_tree_t *right,
>  static reg_errcode_t
>  mark_opt_subexp (void *extra, bin_tree_t *node)
>  {
> -  int idx = (int) (long) extra;
> +  int idx = (int) (intptr_t) extra;
>    if (node->token.type == SUBEXP && node->token.opr.idx == idx)
>      node->token.opt_subexp = 1;
>
>
> base-commit: 4e23cefb4da69a2d884c2d5a303825f40008ca42
> --
> 2.12.2.windows.2.800.gede8f145e06

Let's drop this current gawk import series.

After talking to the gawk author it turns out it's better to use the
version from gnulib, this includes the equivalent of your patch.

The following one-liner works for me on linux to import that library,
on the master branch:

$ git reset --hard; rm compat/regex/*.[ch]; rm -rfv /tmp/git.rx; test
-e /tmp/git-gnulib || git clone
https://git.savannah.gnu.org/git/gnulib.git /tmp/git-gnulib; mkdir
/tmp/git.rx; touch /tmp/git.rx/configure.ac;
/tmp/git-gnulib/gnulib-tool --lgpl --add-import --dir=/tmp/git.rx
regex; cp /tmp/git.rx/lib/{intprops.h,reg*} compat/regex/; perl -0666
-pi.bak -e 's[compat/regex/regex.o: EXTRA_CPPFLAGS =
\K[^\n]+\n[^\n]+][]s' Makefile; echo '#define _GNU_SOURCE'
>compat/regex/config.h

I.e. remove the existing engine, import new one from gnulib, then wipe
the extra -D flags that exist now, and define _GNU_SOURCE.

Re: [PATCH] compat/regex: fix compilation on Windows

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

Hi Ævar,

I originally replied in a very verbose manner, going step by step through
the "one-liner", but decided to rephrase everything.

So here goes.

On Sat, 13 May 2017, Ævar Arnfjörð Bjarmason wrote:

> Let's drop this current gawk import series.

Well, the reason why you imported the current gawk regex is that we (you?)
decided originally that it'd be easier to go with that one rather than
with the gnulib one (which they friendly-forked). If you switch to gnulib,
I would like to see (in the commit message) the original reason we picked
gawk (which forked gnulib's regex code, after all), and why that reason no
longer applies.

I also would strongly prefer a *non* one-liner, in a commit message that
adds the relevant source code from gawk or gnulib *verbatim*, followed by
patches that adjust the code to Git's needs. That way, a future update can
repeat the commands outlined in the first commit message and then
cherry-pick the subsequent patches.

And remember that you do not need to clone the entire repository. You can
1) fetch into the current one, and 2) use a shallow fetch. Example:

	git fetch --depth 1 http://git.savannah.gnu.org/r/gawk.git \
		gawk-4.1.4

The next commands could be something like

	git read-tree --prefix=compat/regex/ FETCH_HEAD:
	git clean -dfx -- compat/regex/

Oh, and please do not use `master`. If Git is any indication, a tagged
release will most often be a bit more robust than any in-between commit.

Thanks,
Dscho

Re: [PATCH] compat/regex: fix compilation on Windows

[Ævar Arnfjörð Bjarmason]

On Sat, May 13, 2017 at 8:30 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Ævar,
>
> I originally replied in a very verbose manner, going step by step through
> the "one-liner", but decided to rephrase everything.
>
> So here goes.
>
> On Sat, 13 May 2017, Ævar Arnfjörð Bjarmason wrote:
>
>> Let's drop this current gawk import series.
>
> Well, the reason why you imported the current gawk regex is that we (you?)
> decided originally that it'd be easier to go with that one rather than
> with the gnulib one (which they friendly-forked). If you switch to gnulib,
> I would like to see (in the commit message) the original reason we picked
> gawk (which forked gnulib's regex code, after all), and why that reason no
> longer applies.

It was just easier to extract the code from gawk than gnulib at the
time. The thread starting at <20100715220059.GA3312@burratino> has
some details.

> I also would strongly prefer a *non* one-liner, in a commit message that
> adds the relevant source code from gawk or gnulib *verbatim*, followed by
> patches that adjust the code to Git's needs. That way, a future update can
> repeat the commands outlined in the first commit message and then
> cherry-pick the subsequent patches.
>
> And remember that you do not need to clone the entire repository. You can
> 1) fetch into the current one, and 2) use a shallow fetch. Example:
>
>         git fetch --depth 1 http://git.savannah.gnu.org/r/gawk.git \
>                 gawk-4.1.4
>
> The next commands could be something like
>
>         git read-tree --prefix=compat/regex/ FETCH_HEAD:
>         git clean -dfx -- compat/regex/

Sure, if I submit this again I'll update whatever one liner is in
there to import it via git.

> Oh, and please do not use `master`. If Git is any indication, a tagged
> release will most often be a bit more robust than any in-between commit.

The reason to use the master branch of gawk is that since the last
release only one trivial change has been made to it, but the files
were all renamed as well, so it was easier for that one-liner in the
README to use master than the latest release.

If we move to gnulib we'll also use the master branch, because that's
what you're supposed to do according to their docs, they don't make
releases.

Re: [PATCH] compat/regex: fix compilation on Windows

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Let's drop this current gawk import series.
>
> After talking to the gawk author it turns out it's better to use the
> version from gnulib, this includes the equivalent of your patch.

OK.  That may make things simpler ;-)

Thanks.