tr/xdiff-fast-hash generates warnings and breaks tests

tr/xdiff-fast-hash generates warnings and breaks tests

[Øyvind A. Holm]

On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the
tr/xdiff-fast-hash branch introduces compiler warnings and breaks
t/t0020-crlf.sh and maybe later tests:

6942efc (xdiff: load full words in the inner loop of xdl_hash_record)
  Introduces these compiler warnings (formatted for readability):

  [...]
      CC xdiff/xprepare.o
      CC xdiff/xutils.o
  xdiff/xutils.c: In function `has_zero':
  xdiff/xutils.c:290: warning: integer constant is too large for
                      `unsigned long' type
  xdiff/xutils.c:290: warning: integer constant is too large for
                      `unsigned long' type
  xdiff/xutils.c: In function `xdl_hash_record':
  xdiff/xutils.c:345: warning: integer constant is too large for
                      `unsigned long' type
  xdiff/xutils.c:361: warning: integer constant is too large for
                      `unsigned long' type
      CC xdiff/xemit.o
      CC xdiff/xmerge.o
  [...]

6f1af02 (xdiff: choose XDL_FAST_HASH code on sizeof(long) instead of
        __WORDSIZE)

  Breaks these tests in t/t0020-crlf.sh :

  not ok - 12 apply patch (autocrlf=input)
  not ok - 13 apply patch --cached (autocrlf=input)
  not ok - 14 apply patch --index (autocrlf=input)
  not ok - 15 apply patch (autocrlf=true)
  not ok - 16 apply patch --cached (autocrlf=true)
  not ok - 17 apply patch --index (autocrlf=true)
  # failed 6 among 34 test(s)

Some later tests might also fail, haven't tested that.

Reverting those two commits on current master (6a4a482) fixes both
problems. Tried building on two other systems (Ubuntu 10.04.4 LTS and
Ubuntu 10.10), and these problems don't appear there. Some info:

  $ lsb_release -a
    No LSB modules are available.
    Distributor ID: Debian
    Description:    Debian GNU/Linux 6.0.5 (squeeze)
    Release:        6.0.5
    Codename:       squeeze

  $ gcc --version
    gcc (Debian 4.4.5-8) 4.4.5

  $ ldd --version
    ldd (Debian EGLIBC 2.11.3-3) 2.11.3

  $ uname -a
    Linux shell 2.6.32-5-amd64 #1 SMP Mon Jan 16 17:15:00 UTC 2012
    x86_64 GNU/Linux

  First part of /proc/cpuinfo:
    processor       : 0
    vendor_id       : GenuineIntel
    cpu family      : 6
    model           : 23
    model name      : Intel(R) Xeon(R) CPU           X3350  @ 2.66GHz
    stepping        : 7
    cpu MHz         : 2666.649
    cache size      : 6144 KB
    physical id     : 0
    siblings        : 4
    core id         : 0
    cpu cores       : 4
    apicid          : 0
    initial apicid  : 0
    fpu             : yes
    fpu_exception   : yes
    cpuid level     : 10
    wp              : yes
    flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr
                      pge mca cmov pat pse36 clflush dts acpi mmx fxsr
                      sse sse2 ss ht tm pbe syscall nx lm constant_tsc
                      arch_perfmon pebs bts rep_good aperfmperf pni
                      dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16
                      xtpr pdcm sse4_1 lahf_lm tpr_shadow vnmi
                      flexpriority
    bogomips        : 5333.29
    clflush size    : 64
    cache_alignment : 64
    address sizes   : 36 bits physical, 48 bits virtual
    power management:

Cheers,
Øyvind

Re: tr/xdiff-fast-hash generates warnings and breaks tests

[René Scharfe]

Am 17.05.2012 01:31, schrieb Øyvind A. Holm:
> On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the
> tr/xdiff-fast-hash branch introduces compiler warnings and breaks
> t/t0020-crlf.sh and maybe later tests:

What does the following short C program report when run (e.g. put it in 
a file named s.c, then run "gcc -o s s.c" and "./s")?

   #include <stdio.h>
   int main(int argc, const char **argv) {
     printf("%u %u %u\n", sizeof(int), sizeof(long), sizeof(void *));
     return 0;
   }

I suspect you run a 32-bit userland on a 64-bit kernel.

On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with 
XDL_FAST_HASH explicitly set (it's off by default).  It succeeds after 
reverting 6f1af02, though, strangely enough.  No compiler warnings are 
printed in either case.

   $ gcc --version
   gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3

   $ uname -a
   Linux ubuntu 3.2.0-24-generic #37-Ubuntu SMP Wed Apr 25 08:43:52 UTC 
2012 i686 i686 i386 GNU/Linux

Also, here are the measurements for master (v1.7.10.2-520-g6a4a482) 
without XDL_FAST_HASH, and with master minus 6f1af02 plus explicitly set 
XDL_FAST_HASH:

   Test                                 master           reverted+FAST
   ---------------------------------------------------------------------
   4000.1: log -3000 (baseline)         0.08(0.05+0.02)  0.08(0.05+0.02)
   4000.2: log --raw -3000 (tree-only)  0.39(0.34+0.04)  0.39(0.32+0.06)
   4000.3: log -p -3000 (Myers)         1.55(1.43+0.11)  1.43(1.29+0.12)
   4000.4: log -p -3000 --histogram     1.63(1.51+0.10)  1.50(1.35+0.14)
   4000.5: log -p -3000 --patience      1.85(1.71+0.13)  1.73(1.62+0.10)

René

Re: tr/xdiff-fast-hash generates warnings and breaks tests

[Øyvind A. Holm]

On 17 May 2012 09:11, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Am 17.05.2012 01:31, schrieb Øyvind A. Holm:
> > On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the
> > tr/xdiff-fast-hash branch introduces compiler warnings and breaks
> > t/t0020-crlf.sh and maybe later tests:
>
> What does the following short C program report when run (e.g. put it
> in a file named s.c, then run "gcc -o s s.c" and "./s")?
>
>  #include <stdio.h>
>  int main(int argc, const char **argv) {
>    printf("%u %u %u\n", sizeof(int), sizeof(long), sizeof(void *));
>    return 0;
>  }

The result is "4 4 4".

> I suspect you run a 32-bit userland on a 64-bit kernel.

Yes, it looks like that to me, too. FYI, this isn't my computer, but a
login shell at a webhosting provider, so I don't know the exact details
about the installation.

Cheers,
Øyvind

Re: tr/xdiff-fast-hash generates warnings and breaks tests

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 17.05.2012 01:31, schrieb Øyvind A. Holm:
>> On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the
>> tr/xdiff-fast-hash branch introduces compiler warnings and breaks
>> t/t0020-crlf.sh and maybe later tests:
>
> What does the following short C program report when run (e.g. put it
> in a file named s.c, then run "gcc -o s s.c" and "./s")?
>
>   #include <stdio.h>
>   int main(int argc, const char **argv) {
>     printf("%u %u %u\n", sizeof(int), sizeof(long), sizeof(void *));
>     return 0;
>   }
>
> I suspect you run a 32-bit userland on a 64-bit kernel.
>
> On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with
> XDL_FAST_HASH explicitly set (it's off by default).

OK.  So does that indicate at least breakage in the Makefile that
attempts to set XDL_FAST_HASH only on x86_64, mistakenly triggering
on Øyvind's x86 32-bit userland, or did Øyvind manually flipped the
feature on?

It is a separate issue that XDL_FAST_HASH code does not work on 32-bit
systems, even though it is advertised that it only needs to be on
little-endian.

> It succeeds after reverting 6f1af02, though, strangely enough.

It is strange.  I do not see anything glaringly wrong in the conversion
in that commit.  The only difference I see is that count_masked_bytes in
the original used to take unsigned long on 64-bit archs but the updated
one takes signed long, but that on 32-bit archs the function takes
signed long in both versions so it cannot be it.  Stumped...

> Also, here are the measurements for master (v1.7.10.2-520-g6a4a482)
> without XDL_FAST_HASH, and with master minus 6f1af02 plus explicitly
> set XDL_FAST_HASH:
>
>   Test                                 master           reverted+FAST
>   ---------------------------------------------------------------------
>   4000.1: log -3000 (baseline)         0.08(0.05+0.02)  0.08(0.05+0.02)
>   4000.2: log --raw -3000 (tree-only)  0.39(0.34+0.04)  0.39(0.32+0.06)
>   4000.3: log -p -3000 (Myers)         1.55(1.43+0.11)  1.43(1.29+0.12)
>   4000.4: log -p -3000 --histogram     1.63(1.51+0.10)  1.50(1.35+0.14)
>   4000.5: log -p -3000 --patience      1.85(1.71+0.13)  1.73(1.62+0.10)

So the patch does give us slight performance edge, when it works ;-)

Re: tr/xdiff-fast-hash generates warnings and breaks tests

[René Scharfe]

Am 17.05.2012 18:23, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
>> On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with
>> XDL_FAST_HASH explicitly set (it's off by default).
>
> OK.  So does that indicate at least breakage in the Makefile that
> attempts to set XDL_FAST_HASH only on x86_64, mistakenly triggering
> on Øyvind's x86 32-bit userland, or did Øyvind manually flipped the
> feature on?

It's turned on by default if uname -m says x86_64, which it does for 
Øyvind (64-bit kernel).  His userland is a 32-bit one, though.

> It is a separate issue that XDL_FAST_HASH code does not work on 32-bit
> systems, even though it is advertised that it only needs to be on
> little-endian.

Indeed.

>> It succeeds after reverting 6f1af02, though, strangely enough.
>
> It is strange.  I do not see anything glaringly wrong in the conversion
> in that commit.  The only difference I see is that count_masked_bytes in
> the original used to take unsigned long on 64-bit archs but the updated
> one takes signed long, but that on 32-bit archs the function takes
> signed long in both versions so it cannot be it.  Stumped...

The assembly differs in two instructions:

	--- master/xdiff/xutils.s
	+++ reverted/xdiff/xutils.s
	@@ -733,7 +733,7 @@
	 	shrl	$7, %ebx
	 	leal	-256(%ebx), %ecx
	 	movl	%ebx, %edi
	-	shrl	$23, %ecx
	+	sarl	$23, %ecx
	 	andl	$1, %edi
	 	leal	1(%ecx,%edi), %ecx
	 	addl	%ecx, %esi
	@@ -934,7 +934,7 @@
	 	jmp	.L123
	 .L153:
	 	movl	$__PRETTY_FUNCTION__.4151, 12(%esp)
	-	movl	$374, 8(%esp)
	+	movl	$380, 8(%esp)
	 	movl	$.LC1, 4(%esp)
	 	movl	$.LC2, (%esp)
	 	call	__assert_fail

The second one is just a line number of an assert() that is moved around 
a bit.  The first one means that master is doing a logical shift right 
(shr) while with 6f1af02 reverted, an arithmetic shift right (sar) is 
performed in the same place.

While sar keeps the sign bit of the operand intact, shr shifts it to the 
right like the other bits.  The code seems to rely on arithmetic shift 
being done.  It's implementation-defined for signed numbers, but that's 
not that much of a problem, as we turn on the feature only on selected 
architectures anyway (modulo detection problems, as above ;).

The real issue seems to be that the shifted number is unsigned:

		long a = (mask - 256) >> 23;

For unsigned values, a logical shift right is done, always. Not sure why 
wrapping it  in "if (sizeof(long) == 8)" would make a difference at all, 
though.

The following patch on top of master makes the compiler put a sarl in my 
build, and "make -j4 XDL_FAST_HASH=1 test" passes.

Øyvind, does this oneliner help in your case as well?

-- >8 --
Subject: xdiff: signed right shift for 32-bit case of XDL_FAST_HASH

In the 32-bit branch of count_masked_bytes(), the compiler uses a
logical right shift on mask, because it is unsigned.  We want it to
be an arithmetic right shift, however (keeping negative numbers
negative instead of shifting the sign bit as well).

There is no way to specify it in C, but we can at least cast mask to
signed long.  This will make the compiler use an arithmetic right shift
on certain implementations, but that's OK because we only turn on
XDL_FAST_HASH in the Makefile for known-good architectures, and at least 
gcc 4.6.3 and clang 3.0 do what we want on the most interesting one (x86).

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
  xdiff/xutils.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 1b3b471..29df57e 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -311,7 +311,7 @@ static inline long count_masked_bytes(unsigned
  		 * (a+b+1) gives us
  		 *   correct 0-3 bytemask count result
  		 */
-		long a = (mask - 256) >> 23;
+		long a = ((long)mask - 256) >> 23;
  		long b = mask & 1;
  		return a + b + 1;
  	}
-- 
1.7.10.2

Re: tr/xdiff-fast-hash generates warnings and breaks tests

[Øyvind A. Holm]

On 17 May 2012 18:23, Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> > On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with
> > XDL_FAST_HASH explicitly set (it's off by default).
>
> OK.  So does that indicate at least breakage in the Makefile that
> attempts to set XDL_FAST_HASH only on x86_64, mistakenly triggering on
> Øyvind's x86 32-bit userland, or did Øyvind manually flipped the
> feature on?

No, I haven't fiddled with any of those, only using standard ./configure
and also verified by running make using the standard Makefile.

On 17 May 2012 20:40, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> The following patch on top of master makes the compiler put a sarl in
> my build, and "make -j4 XDL_FAST_HASH=1 test" passes.
>
> Øyvind, does this oneliner help in your case as well?
>
> Subject: xdiff: signed right shift for 32-bit case of XDL_FAST_HASH
> [...]
>  xdiff/xutils.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 1b3b471..29df57e 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -311,7 +311,7 @@ static inline long count_masked_bytes(unsigned
>                 * (a+b+1) gives us
>                 *   correct 0-3 bytemask count result
>                 */
> -               long a = (mask - 256) >> 23;
> +               long a = ((long)mask - 256) >> 23;
>                long b = mask & 1;
>                return a + b + 1;
>        }

Yes, applied your patch to current master (9de9681), and the whole test
suite runs without errors. The compiler still generates the same
warnings, though:

    CC xdiff/xdiffi.o
    CC xdiff/xprepare.o
    CC xdiff/xutils.o
xdiff/xutils.c: In function ‘has_zero’:
xdiff/xutils.c:261: warning: integer constant is too large for ‘unsigned
                    long’ type
xdiff/xutils.c:261: warning: integer constant is too large for ‘unsigned
                    long’ type
xdiff/xutils.c: In function ‘count_masked_bytes’:
xdiff/xutils.c:273: warning: integer constant is too large for ‘long’
                    type
xdiff/xutils.c: In function ‘xdl_hash_record’:
xdiff/xutils.c:310: warning: integer constant is too large for ‘unsigned
                    long’ type
xdiff/xutils.c:326: warning: integer constant is too large for ‘unsigned
                    long’ type
    CC xdiff/xemit.o
    CC xdiff/xmerge.o

But with this patch, you're certainly on the right track. Thanks.

Cheers,
Øyvind

[PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines

[René Scharfe]

Import macro REPEAT_BYTE from Linux (arch/x86/include/asm/word-at-a-time.h)
to avoid 64-bit integer literals, which cause some 32-bit compilers to
print warnings.

Reported-by: Øyvind A. Holm <sunny@sunbase.org>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 xdiff/xutils.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index ae6ce0d..8580da7 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -251,9 +251,11 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
 
 #ifdef XDL_FAST_HASH
 
-#define ONEBYTES	0x0101010101010101ul
-#define NEWLINEBYTES	0x0a0a0a0a0a0a0a0aul
-#define HIGHBITS	0x8080808080808080ul
+#define REPEAT_BYTE(x)  ((~0ul / 0xff) * (x))
+
+#define ONEBYTES	REPEAT_BYTE(0x01)
+#define NEWLINEBYTES	REPEAT_BYTE(0x0a)
+#define HIGHBITS	REPEAT_BYTE(0x80)
 
 /* Return the high bit set in the first byte that is a zero */
 static inline unsigned long has_zero(unsigned long a)
-- 
1.7.10.2

[PATCH 2/3] xdiff: avoid more compiler warnings with XDL_FAST_HASH on 32-bit machines

[René Scharfe]

Hide literals that can cause compiler warnings for 32-bit architectures in
expressions that evaluate to small numbers there.  Some compilers warn that
0x0001020304050608 won't fit into a 32-bit long, others that shifting right
by 56 bits clears a 32-bit value completely.

The correct values are calculated in the 64-bit case, which is all that matters
in this if-branch.

Reported-by: Øyvind A. Holm <sunny@sunbase.org>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 xdiff/xutils.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 8580da7..78549e3 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -272,7 +272,13 @@ static inline long count_masked_bytes(unsigned long mask)
 		 * that works for the bytemasks without having to
 		 * mask them first.
 		 */
-		return mask * 0x0001020304050608 >> 56;
+		/*
+		 * return mask * 0x0001020304050608 >> 56;
+		 *
+		 * Doing it like this avoids warnings on 32-bit machines.
+		 */
+		long a = (REPEAT_BYTE(0x01) / 0xff + 1);
+		return mask * a >> (sizeof(long) * 7);
 	} else {
 		/*
 		 * Modified Carl Chatfield G+ version for 32-bit *
-- 
1.7.10.2

Re: [PATCH 2/3] xdiff: avoid more compiler warnings with XDL_FAST_HASH on 32-bit machines

[Thomas Rast]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Hide literals that can cause compiler warnings for 32-bit architectures in
> expressions that evaluate to small numbers there.  Some compilers warn that
> 0x0001020304050608 won't fit into a 32-bit long, others that shifting right
> by 56 bits clears a 32-bit value completely.
>
> The correct values are calculated in the 64-bit case, which is all that matters
> in this if-branch.
>
> Reported-by: Øyvind A. Holm <sunny@sunbase.org>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

Thanks for fixing this.  As far as logic and review goes, both patches

Acked-by: Thomas Rast <trast@student.ethz.ch>

I haven't checked whether it actually fixes the warnings, however.

> ---
>  xdiff/xutils.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 8580da7..78549e3 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -272,7 +272,13 @@ static inline long count_masked_bytes(unsigned long mask)
>  		 * that works for the bytemasks without having to
>  		 * mask them first.
>  		 */
> -		return mask * 0x0001020304050608 >> 56;
> +		/*
> +		 * return mask * 0x0001020304050608 >> 56;
> +		 *
> +		 * Doing it like this avoids warnings on 32-bit machines.
> +		 */
> +		long a = (REPEAT_BYTE(0x01) / 0xff + 1);
> +		return mask * a >> (sizeof(long) * 7);
>  	} else {
>  		/*
>  		 * Modified Carl Chatfield G+ version for 32-bit *

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes()

[René Scharfe]

Import the latest 32-bit implementation of count_masked_bytes() from
Linux (arch/x86/include/asm/word-at-a-time.h).  It's shorter and avoids
overflows and negative numbers.

This fixes test failures on 32-bit, where negative partial results had
been shifted right using the "wrong" method (logical shift right instead
of arithmetic short right).  The compiler is free to chose the method,
so it was only wrong in the sense that it didn't work as intended by us.

Reported-by: Øyvind A. Holm <sunny@sunbase.org>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Does this fix all the warnings, and do the tests pass?  I can only
reproduce the "shifting to far to the right" warning..

 xdiff/xutils.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 78549e3..9504eae 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -280,19 +280,11 @@ static inline long count_masked_bytes(unsigned long mask)
 		long a = (REPEAT_BYTE(0x01) / 0xff + 1);
 		return mask * a >> (sizeof(long) * 7);
 	} else {
-		/*
-		 * Modified Carl Chatfield G+ version for 32-bit *
-		 *
-		 * (a) gives us
-		 *   -1 (0, ff), 0 (ffff) or 1 (ffffff)
-		 * (b) gives us
-		 *   0 for 0, 1 for (ff ffff ffffff)
-		 * (a+b+1) gives us
-		 *   correct 0-3 bytemask count result
-		 */
-		long a = (mask - 256) >> 23;
-		long b = mask & 1;
-		return a + b + 1;
+		/* Carl Chatfield / Jan Achrenius G+ version for 32-bit */
+		/* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */
+		long a = (0x0ff0001 + mask) >> 23;
+		/* Fix the 1 for 00 case */
+		return a & mask;
 	}
 }
 
-- 
1.7.10.2

Re: [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes()

[Øyvind A. Holm]

On 22 May 2012 22:36, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Import the latest 32-bit implementation of count_masked_bytes() from
> Linux (arch/x86/include/asm/word-at-a-time.h).  It's shorter and avoids
> overflows and negative numbers.
>
> This fixes test failures on 32-bit, where negative partial results had
> been shifted right using the "wrong" method (logical shift right instead
> of arithmetic short right).  The compiler is free to chose the method,
> so it was only wrong in the sense that it didn't work as intended by us.
>
> Reported-by: Øyvind A. Holm <sunny@sunbase.org>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
> Does this fix all the warnings, and do the tests pass?  I can only
> reproduce the "shifting to far to the right" warning..

Hi, I finally got around to test this, and it compiles flawlessly with
your three patches. No warnings, and the test suite seems to run
without errors.

Nice work, sir.

Cheers,
Øyvind