* tr/xdiff-fast-hash generates warnings and breaks tests
@ 2012-05-16 23:31 Øyvind A. Holm
2012-05-17 7:11 ` René Scharfe
0 siblings, 1 reply; 11+ messages in thread
From: Øyvind A. Holm @ 2012-05-16 23:31 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Junio C Hamano
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: tr/xdiff-fast-hash generates warnings and breaks tests
2012-05-16 23:31 tr/xdiff-fast-hash generates warnings and breaks tests Øyvind A. Holm
@ 2012-05-17 7:11 ` René Scharfe
2012-05-17 9:33 ` Øyvind A. Holm
2012-05-17 16:23 ` Junio C Hamano
0 siblings, 2 replies; 11+ messages in thread
From: René Scharfe @ 2012-05-17 7:11 UTC (permalink / raw)
To: "Øyvind A. Holm"; +Cc: git, Thomas Rast, Junio C Hamano
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é
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: tr/xdiff-fast-hash generates warnings and breaks tests
2012-05-17 7:11 ` René Scharfe
@ 2012-05-17 9:33 ` Øyvind A. Holm
2012-05-17 16:23 ` Junio C Hamano
1 sibling, 0 replies; 11+ messages in thread
From: Øyvind A. Holm @ 2012-05-17 9:33 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Thomas Rast, Junio C Hamano
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: tr/xdiff-fast-hash generates warnings and breaks tests
2012-05-17 7:11 ` René Scharfe
2012-05-17 9:33 ` Øyvind A. Holm
@ 2012-05-17 16:23 ` Junio C Hamano
2012-05-17 18:40 ` René Scharfe
1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2012-05-17 16:23 UTC (permalink / raw)
To: René Scharfe; +Cc: Øyvind A. Holm, git, Thomas Rast, 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 ;-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: tr/xdiff-fast-hash generates warnings and breaks tests
2012-05-17 16:23 ` Junio C Hamano
@ 2012-05-17 18:40 ` René Scharfe
2012-05-19 14:17 ` Øyvind A. Holm
0 siblings, 1 reply; 11+ messages in thread
From: René Scharfe @ 2012-05-17 18:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: "Øyvind A. Holm", git, Thomas Rast
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: tr/xdiff-fast-hash generates warnings and breaks tests
2012-05-17 18:40 ` René Scharfe
@ 2012-05-19 14:17 ` Øyvind A. Holm
2012-05-22 20:36 ` [PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines René Scharfe
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Øyvind A. Holm @ 2012-05-19 14:17 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git, Thomas Rast
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines
2012-05-19 14:17 ` Øyvind A. Holm
@ 2012-05-22 20:36 ` René Scharfe
2012-05-22 20:36 ` [PATCH 2/3] xdiff: avoid more " René Scharfe
2012-05-22 20:36 ` [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes() René Scharfe
2 siblings, 0 replies; 11+ messages in thread
From: René Scharfe @ 2012-05-22 20:36 UTC (permalink / raw)
To: "Øyvind A. Holm"; +Cc: Junio C Hamano, git, Thomas Rast
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] xdiff: avoid more compiler warnings with XDL_FAST_HASH on 32-bit machines
2012-05-19 14:17 ` Øyvind A. Holm
2012-05-22 20:36 ` [PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines René Scharfe
@ 2012-05-22 20:36 ` René Scharfe
2012-05-23 8:30 ` Thomas Rast
2012-05-22 20:36 ` [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes() René Scharfe
2 siblings, 1 reply; 11+ messages in thread
From: René Scharfe @ 2012-05-22 20:36 UTC (permalink / raw)
To: "Øyvind A. Holm"; +Cc: Junio C Hamano, git, Thomas Rast
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes()
2012-05-19 14:17 ` Øyvind A. Holm
2012-05-22 20:36 ` [PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines René Scharfe
2012-05-22 20:36 ` [PATCH 2/3] xdiff: avoid more " René Scharfe
@ 2012-05-22 20:36 ` René Scharfe
2012-05-25 15:18 ` Øyvind A. Holm
2 siblings, 1 reply; 11+ messages in thread
From: René Scharfe @ 2012-05-22 20:36 UTC (permalink / raw)
To: "Øyvind A. Holm"; +Cc: Junio C Hamano, git, Thomas Rast
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] xdiff: avoid more compiler warnings with XDL_FAST_HASH on 32-bit machines
2012-05-22 20:36 ` [PATCH 2/3] xdiff: avoid more " René Scharfe
@ 2012-05-23 8:30 ` Thomas Rast
0 siblings, 0 replies; 11+ messages in thread
From: Thomas Rast @ 2012-05-23 8:30 UTC (permalink / raw)
To: René Scharfe; +Cc: Øyvind A. Holm, Junio C Hamano, git, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes()
2012-05-22 20:36 ` [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes() René Scharfe
@ 2012-05-25 15:18 ` Øyvind A. Holm
0 siblings, 0 replies; 11+ messages in thread
From: Øyvind A. Holm @ 2012-05-25 15:18 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git, Thomas Rast
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-05-25 15:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-16 23:31 tr/xdiff-fast-hash generates warnings and breaks tests Øyvind A. Holm
2012-05-17 7:11 ` René Scharfe
2012-05-17 9:33 ` Øyvind A. Holm
2012-05-17 16:23 ` Junio C Hamano
2012-05-17 18:40 ` René Scharfe
2012-05-19 14:17 ` Øyvind A. Holm
2012-05-22 20:36 ` [PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines René Scharfe
2012-05-22 20:36 ` [PATCH 2/3] xdiff: avoid more " René Scharfe
2012-05-23 8:30 ` Thomas Rast
2012-05-22 20:36 ` [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes() René Scharfe
2012-05-25 15:18 ` Øyvind A. Holm
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).