[PATCH] 64bit issue in test-parse-options.c

[PATCH] 64bit issue in test-parse-options.c

[H.Merijn Brand]

git-1.5.6.4 - HP-UX 11.23 64bit compile

* expecting success:
        test-parse-options -s123 -b -i 1729 -b -vv -n > output 2> output.err &&
        test_cmp expect output &&
        test ! -s output.err

--- expect      2008-07-30 11:52:05 +0000
+++ output      2008-07-30 11:52:05 +0000
@@ -1,5 +1,5 @@
 boolean: 2
-integer: 1729
+integer: 7425998454784
 string: 123
 abbrev: 7
 verbose: 2
* FAIL 2: short options

I'm sure you can come up with a more sensible change, but the current
code is definitely wrong


--8<---
--- test-parse-options.c.org    2008-07-30 11:57:16 +0000
+++ test-parse-options.c        2008-07-30 12:08:56 +0000
@@ -2,6 +2,7 @@
 #include "parse-options.h"

 static int boolean = 0;
+static unsigned int int_integer = 0;
 static unsigned long integer = 0;
 static int abbrev = 7;
 static int verbose = 0, dry_run = 0, quiet = 0;
@@ -29,9 +30,9 @@ int main(int argc, const char **argv)
                OPT_BIT('4', "or4", &boolean,
                        "bitwise-or boolean with ...0100", 4),
                OPT_GROUP(""),
-               OPT_INTEGER('i', "integer", &integer, "get a integer"),
-               OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
-               OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
+               OPT_INTEGER('i', "integer", &int_integer, "get a integer"),
+               OPT_INTEGER('j', NULL, &int_integer, "get a integer, too"),
+               OPT_SET_INT(0, "set23", &int_integer, "set integer to 23", 23),
                OPT_DATE('t', NULL, &integer, "get timestamp of <time>"),
                OPT_CALLBACK('L', "length", &integer, "str",
                        "get length of <str>", length_callback),
@@ -53,7 +54,9 @@ int main(int argc, const char **argv)
        };
        int i;

+       integer = 0x12345678;
        argc = parse_options(argc, argv, options, usage, 0);
+       if (integer == 0x12345678) integer = int_integer;

        printf("boolean: %d\n", boolean);
        printf("integer: %lu\n", integer);
-->8---

-- 
H.Merijn Brand          Amsterdam Perl Mongers  http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, SuSE 10.1, 10.2, and 10.3, AIX 5.2, and Cygwin.
http://mirrors.develooper.com/hpux/           http://www.test-smoke.org/
http://qa.perl.org      http://www.goldmark.org/jeff/stupid-disclaimers/

Re: [PATCH] 64bit issue in test-parse-options.c

[Pierre Habouzit]

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

On Wed, Jul 30, 2008 at 12:16:56PM +0000, H.Merijn Brand wrote:
> git-1.5.6.4 - HP-UX 11.23 64bit compile
> 
> * expecting success:
>         test-parse-options -s123 -b -i 1729 -b -vv -n > output 2> output.err &&
>         test_cmp expect output &&
>         test ! -s output.err
> 
> --- expect      2008-07-30 11:52:05 +0000
> +++ output      2008-07-30 11:52:05 +0000
> @@ -1,5 +1,5 @@
>  boolean: 2
> -integer: 1729
> +integer: 7425998454784
>  string: 123
>  abbrev: 7
>  verbose: 2
> * FAIL 2: short options
> 
> I'm sure you can come up with a more sensible change, but the current
> code is definitely wrong
> 
> 
> --8<---
> --- test-parse-options.c.org    2008-07-30 11:57:16 +0000
> +++ test-parse-options.c        2008-07-30 12:08:56 +0000
> @@ -2,6 +2,7 @@
>  #include "parse-options.h"
> 
>  static int boolean = 0;
> +static unsigned int int_integer = 0;
>  static unsigned long integer = 0;

  long is wrong in the first place, parse-opt only uses ints.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] 64bit issue in test-parse-options.c

[H.Merijn Brand]

On Wed, 30 Jul 2008 14:37:13 +0200, Pierre Habouzit
<madcoder@debian.org> wrote:

> On Wed, Jul 30, 2008 at 12:16:56PM +0000, H.Merijn Brand wrote:
> > git-1.5.6.4 - HP-UX 11.23 64bit compile
> > 
> > * expecting success:
> >         test-parse-options -s123 -b -i 1729 -b -vv -n > output 2> output.err &&
> >         test_cmp expect output &&
> >         test ! -s output.err
> > 
> > --- expect      2008-07-30 11:52:05 +0000
> > +++ output      2008-07-30 11:52:05 +0000
> > @@ -1,5 +1,5 @@
> >  boolean: 2
> > -integer: 1729
> > +integer: 7425998454784
> >  string: 123
> >  abbrev: 7
> >  verbose: 2
> > * FAIL 2: short options
> > 
> > I'm sure you can come up with a more sensible change, but the current
> > code is definitely wrong
> > 
> > 
> > --8<---
> > --- test-parse-options.c.org    2008-07-30 11:57:16 +0000
> > +++ test-parse-options.c        2008-07-30 12:08:56 +0000
> > @@ -2,6 +2,7 @@
> >  #include "parse-options.h"
> > 
> >  static int boolean = 0;
> > +static unsigned int int_integer = 0;
> >  static unsigned long integer = 0;
> 
>   long is wrong in the first place, parse-opt only uses ints.

If I change it to int, the date parsing goes bogus.

-- 
H.Merijn Brand          Amsterdam Perl Mongers  http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, SuSE 10.1, 10.2, and 10.3, AIX 5.2, and Cygwin.
http://mirrors.develooper.com/hpux/           http://www.test-smoke.org/
http://qa.perl.org      http://www.goldmark.org/jeff/stupid-disclaimers/

Re: [PATCH] 64bit issue in test-parse-options.c

[Pierre Habouzit]

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

On Wed, Jul 30, 2008 at 12:44:52PM +0000, H.Merijn Brand wrote:
> On Wed, 30 Jul 2008 14:37:13 +0200, Pierre Habouzit
> <madcoder@debian.org> wrote:
> 
> > On Wed, Jul 30, 2008 at 12:16:56PM +0000, H.Merijn Brand wrote:
> > > git-1.5.6.4 - HP-UX 11.23 64bit compile
> > > 
> > > * expecting success:
> > >         test-parse-options -s123 -b -i 1729 -b -vv -n > output 2> output.err &&
> > >         test_cmp expect output &&
> > >         test ! -s output.err
> > > 
> > > --- expect      2008-07-30 11:52:05 +0000
> > > +++ output      2008-07-30 11:52:05 +0000
> > > @@ -1,5 +1,5 @@
> > >  boolean: 2
> > > -integer: 1729
> > > +integer: 7425998454784
> > >  string: 123
> > >  abbrev: 7
> > >  verbose: 2
> > > * FAIL 2: short options
> > > 
> > > I'm sure you can come up with a more sensible change, but the current
> > > code is definitely wrong
> > > 
> > > 
> > > --8<---
> > > --- test-parse-options.c.org    2008-07-30 11:57:16 +0000
> > > +++ test-parse-options.c        2008-07-30 12:08:56 +0000
> > > @@ -2,6 +2,7 @@
> > >  #include "parse-options.h"
> > > 
> > >  static int boolean = 0;
> > > +static unsigned int int_integer = 0;
> > >  static unsigned long integer = 0;
> > 
> >   long is wrong in the first place, parse-opt only uses ints.
> 
> If I change it to int, the date parsing goes bogus.

That is because OPT_DATE indeed expect a long (why not a time_t I don't
know btw... but time_t is a long on linux so it doesn't change things a
lot).

The proper fix is to let integer be an *INT* (long integer is bogus
anyways) and to put the date in a long using static unsigned long date,
and make OPT_DATE use this long.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] 64bit issue in test-parse-options.c

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> The proper fix is to let integer be an *INT* (long integer is bogus
> anyways) and to put the date in a long using static unsigned long date,
> and make OPT_DATE use this long.

I am still puzzled by the original report of the breakage, as H. Merijn
cannot be the first person to ever ran test-parse-options on 64-bit
machine.  Maybe there is a bytesex issue involved as well?

In any case, this should work.

 test-parse-options.c     |    8 +++++---
 t/t0040-parse-options.sh |   11 ++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/test-parse-options.c b/test-parse-options.c
index 2a79e72..6e18083 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -2,7 +2,8 @@
 #include "parse-options.h"
 
 static int boolean = 0;
-static unsigned long integer = 0;
+static int integer = 0;
+static unsigned long timestamp;
 static int abbrev = 7;
 static int verbose = 0, dry_run = 0, quiet = 0;
 static char *string = NULL;
@@ -32,7 +33,7 @@ int main(int argc, const char **argv)
 		OPT_INTEGER('i', "integer", &integer, "get a integer"),
 		OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
 		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
-		OPT_DATE('t', NULL, &integer, "get timestamp of <time>"),
+		OPT_DATE('t', NULL, &timestamp, "get timestamp of <time>"),
 		OPT_CALLBACK('L', "length", &integer, "str",
 			"get length of <str>", length_callback),
 		OPT_GROUP("String options"),
@@ -56,7 +57,8 @@ int main(int argc, const char **argv)
 	argc = parse_options(argc, argv, options, usage, 0);
 
 	printf("boolean: %d\n", boolean);
-	printf("integer: %lu\n", integer);
+	printf("integer: %u\n", integer);
+	printf("timestamp: %lu\n", timestamp);
 	printf("string: %s\n", string ? string : "(not set)");
 	printf("abbrev: %d\n", abbrev);
 	printf("verbose: %d\n", verbose);


diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 03dbe00..e38241c 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -47,6 +47,7 @@ test_expect_success 'test help' '
 cat > expect << EOF
 boolean: 2
 integer: 1729
+timestamp: 0
 string: 123
 abbrev: 7
 verbose: 2
@@ -63,6 +64,7 @@ test_expect_success 'short options' '
 cat > expect << EOF
 boolean: 2
 integer: 1729
+timestamp: 0
 string: 321
 abbrev: 10
 verbose: 2
@@ -88,6 +90,7 @@ test_expect_success 'missing required value' '
 cat > expect << EOF
 boolean: 1
 integer: 13
+timestamp: 0
 string: 123
 abbrev: 7
 verbose: 0
@@ -108,6 +111,7 @@ test_expect_success 'intermingled arguments' '
 cat > expect << EOF
 boolean: 0
 integer: 2
+timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
@@ -135,6 +139,7 @@ test_expect_success 'ambiguously abbreviated option' '
 cat > expect << EOF
 boolean: 0
 integer: 0
+timestamp: 0
 string: 123
 abbrev: 7
 verbose: 0
@@ -161,6 +166,7 @@ test_expect_success 'detect possible typos' '
 cat > expect <<EOF
 boolean: 0
 integer: 0
+timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
@@ -177,7 +183,8 @@ test_expect_success 'keep some options as arguments' '
 
 cat > expect <<EOF
 boolean: 0
-integer: 1
+integer: 0
+timestamp: 1
 string: default
 abbrev: 7
 verbose: 0
@@ -197,6 +204,7 @@ cat > expect <<EOF
 Callback: "four", 0
 boolean: 5
 integer: 4
+timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
@@ -223,6 +231,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
 cat > expect <<EOF
 boolean: 1
 integer: 23
+timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0

Re: [PATCH] 64bit issue in test-parse-options.c

[Pierre Habouzit]

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

On Wed, Jul 30, 2008 at 07:11:35PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > The proper fix is to let integer be an *INT* (long integer is bogus
> > anyways) and to put the date in a long using static unsigned long date,
> > and make OPT_DATE use this long.
> 
> I am still puzzled by the original report of the breakage, as H. Merijn
> cannot be the first person to ever ran test-parse-options on 64-bit
> machine.  Maybe there is a bytesex issue involved as well?

  Yes there is: I assume HP-UX works on big endian machines, else on
machines where longs are 64 bits, we put timestamps in it which are
probably current dates, which still fit on 31 (or 32) bits, hence do not
touch the most significant 32 bits hence work on 64 bits little endian
machines (like ... x86_64).

  So when we parse an integer in it again, the 32 most significant bits
remain 0 hence the test passes.

> In any case, this should work.
> 
>  test-parse-options.c     |    8 +++++---
>  t/t0040-parse-options.sh |   11 ++++++++++-
>  2 files changed, 15 insertions(+), 4 deletions(-)

  Ack. I wanted to write it but hadn't the time yet, I was just pointing
to the right fix. Thanks for writing the patch :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] 64bit issue in test-parse-options.c

[H.Merijn Brand]

On Wed, 30 Jul 2008 12:11:35 -0700, Junio C Hamano <gitster@pobox.com>
wrote:

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > The proper fix is to let integer be an *INT* (long integer is bogus
> > anyways) and to put the date in a long using static unsigned long date,
> > and make OPT_DATE use this long.
> 
> I am still puzzled by the original report of the breakage, as H. Merijn
> cannot be the first person to ever ran test-parse-options on 64-bit
> machine.  Maybe there is a bytesex issue involved as well?

Itanium 2 / 64, and endianness indeed

> In any case, this should work.

Yes, that was what I was thinking too, but I had no time to go for the
test cases too. I had it to get to work ASAP.

>  test-parse-options.c     |    8 +++++---
>  t/t0040-parse-options.sh |   11 ++++++++++-
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/test-parse-options.c b/test-parse-options.c
> index 2a79e72..6e18083 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -2,7 +2,8 @@
>  #include "parse-options.h"
>  
>  static int boolean = 0;
> -static unsigned long integer = 0;
> +static int integer = 0;
> +static unsigned long timestamp;
>  static int abbrev = 7;
>  static int verbose = 0, dry_run = 0, quiet = 0;
>  static char *string = NULL;
> @@ -32,7 +33,7 @@ int main(int argc, const char **argv)
>  		OPT_INTEGER('i', "integer", &integer, "get a integer"),
>  		OPT_INTEGER('j', NULL, &integer, "get a integer, too"),
>  		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
> -		OPT_DATE('t', NULL, &integer, "get timestamp of <time>"),
> +		OPT_DATE('t', NULL, &timestamp, "get timestamp of <time>"),
>  		OPT_CALLBACK('L', "length", &integer, "str",
>  			"get length of <str>", length_callback),
>  		OPT_GROUP("String options"),
> @@ -56,7 +57,8 @@ int main(int argc, const char **argv)
>  	argc = parse_options(argc, argv, options, usage, 0);
>  
>  	printf("boolean: %d\n", boolean);
> -	printf("integer: %lu\n", integer);
> +	printf("integer: %u\n", integer);
> +	printf("timestamp: %lu\n", timestamp);
>  	printf("string: %s\n", string ? string : "(not set)");
>  	printf("abbrev: %d\n", abbrev);
>  	printf("verbose: %d\n", verbose);
> 
> 
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 03dbe00..e38241c 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -47,6 +47,7 @@ test_expect_success 'test help' '
>  cat > expect << EOF
>  boolean: 2
>  integer: 1729
> +timestamp: 0
>  string: 123
>  abbrev: 7
>  verbose: 2
> @@ -63,6 +64,7 @@ test_expect_success 'short options' '
>  cat > expect << EOF
>  boolean: 2
>  integer: 1729
> +timestamp: 0
>  string: 321
>  abbrev: 10
>  verbose: 2
> @@ -88,6 +90,7 @@ test_expect_success 'missing required value' '
>  cat > expect << EOF
>  boolean: 1
>  integer: 13
> +timestamp: 0
>  string: 123
>  abbrev: 7
>  verbose: 0
> @@ -108,6 +111,7 @@ test_expect_success 'intermingled arguments' '
>  cat > expect << EOF
>  boolean: 0
>  integer: 2
> +timestamp: 0
>  string: (not set)
>  abbrev: 7
>  verbose: 0
> @@ -135,6 +139,7 @@ test_expect_success 'ambiguously abbreviated option' '
>  cat > expect << EOF
>  boolean: 0
>  integer: 0
> +timestamp: 0
>  string: 123
>  abbrev: 7
>  verbose: 0
> @@ -161,6 +166,7 @@ test_expect_success 'detect possible typos' '
>  cat > expect <<EOF
>  boolean: 0
>  integer: 0
> +timestamp: 0
>  string: (not set)
>  abbrev: 7
>  verbose: 0
> @@ -177,7 +183,8 @@ test_expect_success 'keep some options as arguments' '
>  
>  cat > expect <<EOF
>  boolean: 0
> -integer: 1
> +integer: 0
> +timestamp: 1
>  string: default
>  abbrev: 7
>  verbose: 0
> @@ -197,6 +204,7 @@ cat > expect <<EOF
>  Callback: "four", 0
>  boolean: 5
>  integer: 4
> +timestamp: 0
>  string: (not set)
>  abbrev: 7
>  verbose: 0
> @@ -223,6 +231,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
>  cat > expect <<EOF
>  boolean: 1
>  integer: 23
> +timestamp: 0
>  string: (not set)
>  abbrev: 7
>  verbose: 0


-- 
H.Merijn Brand          Amsterdam Perl Mongers  http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, SuSE 10.1, 10.2, and 10.3, AIX 5.2, and Cygwin.
http://mirrors.develooper.com/hpux/           http://www.test-smoke.org/
http://qa.perl.org      http://www.goldmark.org/jeff/stupid-disclaimers/

Re: [PATCH] 64bit issue in test-parse-options.c

[Petr Baudis]

On Wed, Jul 30, 2008 at 04:05:23PM +0200, Pierre Habouzit wrote:
> On Wed, Jul 30, 2008 at 12:44:52PM +0000, H.Merijn Brand wrote:
> > On Wed, 30 Jul 2008 14:37:13 +0200, Pierre Habouzit
> > <madcoder@debian.org> wrote:
> > >   long is wrong in the first place, parse-opt only uses ints.
> > 
> > If I change it to int, the date parsing goes bogus.
> 
> That is because OPT_DATE indeed expect a long (why not a time_t I don't
> know btw... but time_t is a long on linux so it doesn't change things a
> lot).

Still, I think converting to time_t for timestamps would be a good
cleanup; I have added it to http://git.or.cz/gitwiki/Janitor.

				Petr "Pasky" Baudis