{"thread":{"id":"30543","subject":"tr/xdiff-fast-hash generates warnings and breaks tests","startedAt":"2012-05-16T23:31:54Z","lastAt":"2012-05-25T15:18:00Z","messageCount":11,"participants":["Øyvind A. Holm","René Scharfe","Junio C Hamano","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"191615","messageId":"CAA787r=WCJXeDipiVL37oMgji=ncoPyXXVOcCyYbSC6iCcTi1g@mail.gmail.com","threadId":"30543","inReplyTo":null,"subject":"tr/xdiff-fast-hash generates warnings and breaks tests","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2012-05-16T23:31:54Z","receivedAt":"2012-05-16T23:31:54Z","isPatch":false,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the\ntr/xdiff-fast-hash branch introduces compiler warnings and breaks\nt/t0020-crlf.sh and maybe later tests:\n\n6942efc (xdiff: load full words in the inner loop of xdl_hash_record)\n  Introduces these compiler warnings (formatted for readability):\n\n  [...]\n      CC xdiff/xprepare.o\n      CC xdiff/xutils.o\n  xdiff/xutils.c: In function `has_zero':\n  xdiff/xutils.c:290: warning: integer constant is too large for\n                      `unsigned long' type\n  xdiff/xutils.c:290: warning: integer constant is too large for\n                      `unsigned long' type\n  xdiff/xutils.c: In function `xdl_hash_record':\n  xdiff/xutils.c:345: warning: integer constant is too large for\n                      `unsigned long' type\n  xdiff/xutils.c:361: warning: integer constant is too large for\n                      `unsigned long' type\n      CC xdiff/xemit.o\n      CC xdiff/xmerge.o\n  [...]\n\n6f1af02 (xdiff: choose XDL_FAST_HASH code on sizeof(long) instead of\n        __WORDSIZE)\n\n  Breaks these tests in t/t0020-crlf.sh :\n\n  not ok - 12 apply patch (autocrlf=input)\n  not ok - 13 apply patch --cached (autocrlf=input)\n  not ok - 14 apply patch --index (autocrlf=input)\n  not ok - 15 apply patch (autocrlf=true)\n  not ok - 16 apply patch --cached (autocrlf=true)\n  not ok - 17 apply patch --index (autocrlf=true)\n  # failed 6 among 34 test(s)\n\nSome later tests might also fail, haven't tested that.\n\nReverting those two commits on current master (6a4a482) fixes both\nproblems. Tried building on two other systems (Ubuntu 10.04.4 LTS and\nUbuntu 10.10), and these problems don't appear there. Some info:\n\n  $ lsb_release -a\n    No LSB modules are available.\n    Distributor ID: Debian\n    Description:    Debian GNU/Linux 6.0.5 (squeeze)\n    Release:        6.0.5\n    Codename:       squeeze\n\n  $ gcc --version\n    gcc (Debian 4.4.5-8) 4.4.5\n\n  $ ldd --version\n    ldd (Debian EGLIBC 2.11.3-3) 2.11.3\n\n  $ uname -a\n    Linux shell 2.6.32-5-amd64 #1 SMP Mon Jan 16 17:15:00 UTC 2012\n    x86_64 GNU/Linux\n\n  First part of /proc/cpuinfo:\n    processor       : 0\n    vendor_id       : GenuineIntel\n    cpu family      : 6\n    model           : 23\n    model name      : Intel(R) Xeon(R) CPU           X3350  @ 2.66GHz\n    stepping        : 7\n    cpu MHz         : 2666.649\n    cache size      : 6144 KB\n    physical id     : 0\n    siblings        : 4\n    core id         : 0\n    cpu cores       : 4\n    apicid          : 0\n    initial apicid  : 0\n    fpu             : yes\n    fpu_exception   : yes\n    cpuid level     : 10\n    wp              : yes\n    flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr\n                      pge mca cmov pat pse36 clflush dts acpi mmx fxsr\n                      sse sse2 ss ht tm pbe syscall nx lm constant_tsc\n                      arch_perfmon pebs bts rep_good aperfmperf pni\n                      dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16\n                      xtpr pdcm sse4_1 lahf_lm tpr_shadow vnmi\n                      flexpriority\n    bogomips        : 5333.29\n    clflush size    : 64\n    cache_alignment : 64\n    address sizes   : 36 bits physical, 48 bits virtual\n    power management:\n\nCheers,\nØyvind\n"},{"id":"191621","messageId":"4FB4A4B9.3080009@lsrfire.ath.cx","threadId":"30543","inReplyTo":"CAA787r=WCJXeDipiVL37oMgji=ncoPyXXVOcCyYbSC6iCcTi1g@mail.gmail.com","subject":"Re: tr/xdiff-fast-hash generates warnings and breaks tests","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-17T07:11:53Z","receivedAt":"2012-05-17T07:11:53Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.05.2012 01:31, schrieb Øyvind A. Holm:\n> On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the\n> tr/xdiff-fast-hash branch introduces compiler warnings and breaks\n> t/t0020-crlf.sh and maybe later tests:\n\nWhat does the following short C program report when run (e.g. put it in \na file named s.c, then run \"gcc -o s s.c\" and \"./s\")?\n\n   #include <stdio.h>\n   int main(int argc, const char **argv) {\n     printf(\"%u %u %u\\n\", sizeof(int), sizeof(long), sizeof(void *));\n     return 0;\n   }\n\nI suspect you run a 32-bit userland on a 64-bit kernel.\n\nOn Ubuntu 12.04 x86, t0020 fails for me as well when I compile with \nXDL_FAST_HASH explicitly set (it's off by default).  It succeeds after \nreverting 6f1af02, though, strangely enough.  No compiler warnings are \nprinted in either case.\n\n   $ gcc --version\n   gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3\n\n   $ uname -a\n   Linux ubuntu 3.2.0-24-generic #37-Ubuntu SMP Wed Apr 25 08:43:52 UTC \n2012 i686 i686 i386 GNU/Linux\n\nAlso, here are the measurements for master (v1.7.10.2-520-g6a4a482) \nwithout XDL_FAST_HASH, and with master minus 6f1af02 plus explicitly set \nXDL_FAST_HASH:\n\n   Test                                 master           reverted+FAST\n   ---------------------------------------------------------------------\n   4000.1: log -3000 (baseline)         0.08(0.05+0.02)  0.08(0.05+0.02)\n   4000.2: log --raw -3000 (tree-only)  0.39(0.34+0.04)  0.39(0.32+0.06)\n   4000.3: log -p -3000 (Myers)         1.55(1.43+0.11)  1.43(1.29+0.12)\n   4000.4: log -p -3000 --histogram     1.63(1.51+0.10)  1.50(1.35+0.14)\n   4000.5: log -p -3000 --patience      1.85(1.71+0.13)  1.73(1.62+0.10)\n\nRené\n"},{"id":"191623","messageId":"CAA787r=aMJCQvxtMZaYUHJsTkxs-mUYBYroArvVN=E=_EGKDDQ@mail.gmail.com","threadId":"30543","inReplyTo":"4FB4A4B9.3080009@lsrfire.ath.cx","subject":"Re: tr/xdiff-fast-hash generates warnings and breaks tests","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2012-05-17T09:33:27Z","receivedAt":"2012-05-17T09:33:27Z","isPatch":false,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"On 17 May 2012 09:11, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:\n> Am 17.05.2012 01:31, schrieb Øyvind A. Holm:\n> > On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the\n> > tr/xdiff-fast-hash branch introduces compiler warnings and breaks\n> > t/t0020-crlf.sh and maybe later tests:\n>\n> What does the following short C program report when run (e.g. put it\n> in a file named s.c, then run \"gcc -o s s.c\" and \"./s\")?\n>\n>  #include <stdio.h>\n>  int main(int argc, const char **argv) {\n>    printf(\"%u %u %u\\n\", sizeof(int), sizeof(long), sizeof(void *));\n>    return 0;\n>  }\n\nThe result is \"4 4 4\".\n\n> I suspect you run a 32-bit userland on a 64-bit kernel.\n\nYes, it looks like that to me, too. FYI, this isn't my computer, but a\nlogin shell at a webhosting provider, so I don't know the exact details\nabout the installation.\n\nCheers,\nØyvind\n"},{"id":"191628","messageId":"xmqqmx56rd2r.fsf@junio.mtv.corp.google.com","threadId":"30543","inReplyTo":"4FB4A4B9.3080009@lsrfire.ath.cx","subject":"Re: tr/xdiff-fast-hash generates warnings and breaks tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-17T16:23:40Z","receivedAt":"2012-05-17T16:23:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Am 17.05.2012 01:31, schrieb Øyvind A. Holm:\n>> On Debian GNU/Linux 6.0.5 (squeeze), the two commits on the\n>> tr/xdiff-fast-hash branch introduces compiler warnings and breaks\n>> t/t0020-crlf.sh and maybe later tests:\n>\n> What does the following short C program report when run (e.g. put it\n> in a file named s.c, then run \"gcc -o s s.c\" and \"./s\")?\n>\n>   #include <stdio.h>\n>   int main(int argc, const char **argv) {\n>     printf(\"%u %u %u\\n\", sizeof(int), sizeof(long), sizeof(void *));\n>     return 0;\n>   }\n>\n> I suspect you run a 32-bit userland on a 64-bit kernel.\n>\n> On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with\n> XDL_FAST_HASH explicitly set (it's off by default).\n\nOK.  So does that indicate at least breakage in the Makefile that\nattempts to set XDL_FAST_HASH only on x86_64, mistakenly triggering\non Øyvind's x86 32-bit userland, or did Øyvind manually flipped the\nfeature on?\n\nIt is a separate issue that XDL_FAST_HASH code does not work on 32-bit\nsystems, even though it is advertised that it only needs to be on\nlittle-endian.\n\n> It succeeds after reverting 6f1af02, though, strangely enough.\n\nIt is strange.  I do not see anything glaringly wrong in the conversion\nin that commit.  The only difference I see is that count_masked_bytes in\nthe original used to take unsigned long on 64-bit archs but the updated\none takes signed long, but that on 32-bit archs the function takes\nsigned long in both versions so it cannot be it.  Stumped...\n\n> Also, here are the measurements for master (v1.7.10.2-520-g6a4a482)\n> without XDL_FAST_HASH, and with master minus 6f1af02 plus explicitly\n> set XDL_FAST_HASH:\n>\n>   Test                                 master           reverted+FAST\n>   ---------------------------------------------------------------------\n>   4000.1: log -3000 (baseline)         0.08(0.05+0.02)  0.08(0.05+0.02)\n>   4000.2: log --raw -3000 (tree-only)  0.39(0.34+0.04)  0.39(0.32+0.06)\n>   4000.3: log -p -3000 (Myers)         1.55(1.43+0.11)  1.43(1.29+0.12)\n>   4000.4: log -p -3000 --histogram     1.63(1.51+0.10)  1.50(1.35+0.14)\n>   4000.5: log -p -3000 --patience      1.85(1.71+0.13)  1.73(1.62+0.10)\n\nSo the patch does give us slight performance edge, when it works ;-)\n"},{"id":"191632","messageId":"4FB5460C.10807@lsrfire.ath.cx","threadId":"30543","inReplyTo":"xmqqmx56rd2r.fsf@junio.mtv.corp.google.com","subject":"Re: tr/xdiff-fast-hash generates warnings and breaks tests","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-17T18:40:12Z","receivedAt":"2012-05-17T18:40:12Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.05.2012 18:23, schrieb Junio C Hamano:\n> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:\n>> On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with\n>> XDL_FAST_HASH explicitly set (it's off by default).\n>\n> OK.  So does that indicate at least breakage in the Makefile that\n> attempts to set XDL_FAST_HASH only on x86_64, mistakenly triggering\n> on Øyvind's x86 32-bit userland, or did Øyvind manually flipped the\n> feature on?\n\nIt's turned on by default if uname -m says x86_64, which it does for \nØyvind (64-bit kernel).  His userland is a 32-bit one, though.\n\n> It is a separate issue that XDL_FAST_HASH code does not work on 32-bit\n> systems, even though it is advertised that it only needs to be on\n> little-endian.\n\nIndeed.\n\n>> It succeeds after reverting 6f1af02, though, strangely enough.\n>\n> It is strange.  I do not see anything glaringly wrong in the conversion\n> in that commit.  The only difference I see is that count_masked_bytes in\n> the original used to take unsigned long on 64-bit archs but the updated\n> one takes signed long, but that on 32-bit archs the function takes\n> signed long in both versions so it cannot be it.  Stumped...\n\nThe assembly differs in two instructions:\n\n\t--- master/xdiff/xutils.s\n\t+++ reverted/xdiff/xutils.s\n\t@@ -733,7 +733,7 @@\n\t \tshrl\t$7, %ebx\n\t \tleal\t-256(%ebx), %ecx\n\t \tmovl\t%ebx, %edi\n\t-\tshrl\t$23, %ecx\n\t+\tsarl\t$23, %ecx\n\t \tandl\t$1, %edi\n\t \tleal\t1(%ecx,%edi), %ecx\n\t \taddl\t%ecx, %esi\n\t@@ -934,7 +934,7 @@\n\t \tjmp\t.L123\n\t .L153:\n\t \tmovl\t$__PRETTY_FUNCTION__.4151, 12(%esp)\n\t-\tmovl\t$374, 8(%esp)\n\t+\tmovl\t$380, 8(%esp)\n\t \tmovl\t$.LC1, 4(%esp)\n\t \tmovl\t$.LC2, (%esp)\n\t \tcall\t__assert_fail\n\nThe second one is just a line number of an assert() that is moved around \na bit.  The first one means that master is doing a logical shift right \n(shr) while with 6f1af02 reverted, an arithmetic shift right (sar) is \nperformed in the same place.\n\nWhile sar keeps the sign bit of the operand intact, shr shifts it to the \nright like the other bits.  The code seems to rely on arithmetic shift \nbeing done.  It's implementation-defined for signed numbers, but that's \nnot that much of a problem, as we turn on the feature only on selected \narchitectures anyway (modulo detection problems, as above ;).\n\nThe real issue seems to be that the shifted number is unsigned:\n\n\t\tlong a = (mask - 256) >> 23;\n\nFor unsigned values, a logical shift right is done, always. Not sure why \nwrapping it  in \"if (sizeof(long) == 8)\" would make a difference at all, \nthough.\n\nThe following patch on top of master makes the compiler put a sarl in my \nbuild, and \"make -j4 XDL_FAST_HASH=1 test\" passes.\n\nØyvind, does this oneliner help in your case as well?\n\n-- >8 --\nSubject: xdiff: signed right shift for 32-bit case of XDL_FAST_HASH\n\nIn the 32-bit branch of count_masked_bytes(), the compiler uses a\nlogical right shift on mask, because it is unsigned.  We want it to\nbe an arithmetic right shift, however (keeping negative numbers\nnegative instead of shifting the sign bit as well).\n\nThere is no way to specify it in C, but we can at least cast mask to\nsigned long.  This will make the compiler use an arithmetic right shift\non certain implementations, but that's OK because we only turn on\nXDL_FAST_HASH in the Makefile for known-good architectures, and at least \ngcc 4.6.3 and clang 3.0 do what we want on the most interesting one (x86).\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n  xdiff/xutils.c |    2 +-\n  1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 1b3b471..29df57e 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -311,7 +311,7 @@ static inline long count_masked_bytes(unsigned\n  \t\t * (a+b+1) gives us\n  \t\t *   correct 0-3 bytemask count result\n  \t\t */\n-\t\tlong a = (mask - 256) >> 23;\n+\t\tlong a = ((long)mask - 256) >> 23;\n  \t\tlong b = mask & 1;\n  \t\treturn a + b + 1;\n  \t}\n-- \n1.7.10.2\n"},{"id":"191737","messageId":"CAA787rmJixvyKhubHXZCDVYc=DdVk8_vHsHF6bOsLQ_j=39bGw@mail.gmail.com","threadId":"30543","inReplyTo":"4FB5460C.10807@lsrfire.ath.cx","subject":"Re: tr/xdiff-fast-hash generates warnings and breaks tests","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2012-05-19T14:17:23Z","receivedAt":"2012-05-19T14:17:23Z","isPatch":false,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"On 17 May 2012 18:23, Junio C Hamano <gitster@pobox.com> wrote:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> > On Ubuntu 12.04 x86, t0020 fails for me as well when I compile with\n> > XDL_FAST_HASH explicitly set (it's off by default).\n>\n> OK.  So does that indicate at least breakage in the Makefile that\n> attempts to set XDL_FAST_HASH only on x86_64, mistakenly triggering on\n> Øyvind's x86 32-bit userland, or did Øyvind manually flipped the\n> feature on?\n\nNo, I haven't fiddled with any of those, only using standard ./configure\nand also verified by running make using the standard Makefile.\n\nOn 17 May 2012 20:40, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:\n> The following patch on top of master makes the compiler put a sarl in\n> my build, and \"make -j4 XDL_FAST_HASH=1 test\" passes.\n>\n> Øyvind, does this oneliner help in your case as well?\n>\n> Subject: xdiff: signed right shift for 32-bit case of XDL_FAST_HASH\n> [...]\n>  xdiff/xutils.c |    2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n> index 1b3b471..29df57e 100644\n> --- a/xdiff/xutils.c\n> +++ b/xdiff/xutils.c\n> @@ -311,7 +311,7 @@ static inline long count_masked_bytes(unsigned\n>                 * (a+b+1) gives us\n>                 *   correct 0-3 bytemask count result\n>                 */\n> -               long a = (mask - 256) >> 23;\n> +               long a = ((long)mask - 256) >> 23;\n>                long b = mask & 1;\n>                return a + b + 1;\n>        }\n\nYes, applied your patch to current master (9de9681), and the whole test\nsuite runs without errors. The compiler still generates the same\nwarnings, though:\n\n    CC xdiff/xdiffi.o\n    CC xdiff/xprepare.o\n    CC xdiff/xutils.o\nxdiff/xutils.c: In function ‘has_zero’:\nxdiff/xutils.c:261: warning: integer constant is too large for ‘unsigned\n                    long’ type\nxdiff/xutils.c:261: warning: integer constant is too large for ‘unsigned\n                    long’ type\nxdiff/xutils.c: In function ‘count_masked_bytes’:\nxdiff/xutils.c:273: warning: integer constant is too large for ‘long’\n                    type\nxdiff/xutils.c: In function ‘xdl_hash_record’:\nxdiff/xutils.c:310: warning: integer constant is too large for ‘unsigned\n                    long’ type\nxdiff/xutils.c:326: warning: integer constant is too large for ‘unsigned\n                    long’ type\n    CC xdiff/xemit.o\n    CC xdiff/xmerge.o\n\nBut with this patch, you're certainly on the right track. Thanks.\n\nCheers,\nØyvind\n"},{"id":"191942","messageId":"4FBBF8C9.7050906@lsrfire.ath.cx","threadId":"30543","inReplyTo":"CAA787rmJixvyKhubHXZCDVYc=DdVk8_vHsHF6bOsLQ_j=39bGw@mail.gmail.com","subject":"[PATCH 1/3] xdiff: avoid compiler warnings with XDL_FAST_HASH on 32-bit machines","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-22T20:36:25Z","receivedAt":"2012-05-22T20:36:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Import macro REPEAT_BYTE from Linux (arch/x86/include/asm/word-at-a-time.h)\nto avoid 64-bit integer literals, which cause some 32-bit compilers to\nprint warnings.\n\nReported-by: Øyvind A. Holm <sunny@sunbase.org>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n xdiff/xutils.c |    8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex ae6ce0d..8580da7 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -251,9 +251,11 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n \n #ifdef XDL_FAST_HASH\n \n-#define ONEBYTES\t0x0101010101010101ul\n-#define NEWLINEBYTES\t0x0a0a0a0a0a0a0a0aul\n-#define HIGHBITS\t0x8080808080808080ul\n+#define REPEAT_BYTE(x)  ((~0ul / 0xff) * (x))\n+\n+#define ONEBYTES\tREPEAT_BYTE(0x01)\n+#define NEWLINEBYTES\tREPEAT_BYTE(0x0a)\n+#define HIGHBITS\tREPEAT_BYTE(0x80)\n \n /* Return the high bit set in the first byte that is a zero */\n static inline unsigned long has_zero(unsigned long a)\n-- \n1.7.10.2\n"},{"id":"191941","messageId":"4FBBF8D7.7050701@lsrfire.ath.cx","threadId":"30543","inReplyTo":"CAA787rmJixvyKhubHXZCDVYc=DdVk8_vHsHF6bOsLQ_j=39bGw@mail.gmail.com","subject":"[PATCH 2/3] xdiff: avoid more compiler warnings with XDL_FAST_HASH on 32-bit machines","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-22T20:36:39Z","receivedAt":"2012-05-22T20:36:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Hide literals that can cause compiler warnings for 32-bit architectures in\nexpressions that evaluate to small numbers there.  Some compilers warn that\n0x0001020304050608 won't fit into a 32-bit long, others that shifting right\nby 56 bits clears a 32-bit value completely.\n\nThe correct values are calculated in the 64-bit case, which is all that matters\nin this if-branch.\n\nReported-by: Øyvind A. Holm <sunny@sunbase.org>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n xdiff/xutils.c |    8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 8580da7..78549e3 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -272,7 +272,13 @@ static inline long count_masked_bytes(unsigned long mask)\n \t\t * that works for the bytemasks without having to\n \t\t * mask them first.\n \t\t */\n-\t\treturn mask * 0x0001020304050608 >> 56;\n+\t\t/*\n+\t\t * return mask * 0x0001020304050608 >> 56;\n+\t\t *\n+\t\t * Doing it like this avoids warnings on 32-bit machines.\n+\t\t */\n+\t\tlong a = (REPEAT_BYTE(0x01) / 0xff + 1);\n+\t\treturn mask * a >> (sizeof(long) * 7);\n \t} else {\n \t\t/*\n \t\t * Modified Carl Chatfield G+ version for 32-bit *\n-- \n1.7.10.2\n"},{"id":"191943","messageId":"4FBBF8E9.7020103@lsrfire.ath.cx","threadId":"30543","inReplyTo":"CAA787rmJixvyKhubHXZCDVYc=DdVk8_vHsHF6bOsLQ_j=39bGw@mail.gmail.com","subject":"[PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-22T20:36:57Z","receivedAt":"2012-05-22T20:36:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Import the latest 32-bit implementation of count_masked_bytes() from\nLinux (arch/x86/include/asm/word-at-a-time.h).  It's shorter and avoids\noverflows and negative numbers.\n\nThis fixes test failures on 32-bit, where negative partial results had\nbeen shifted right using the \"wrong\" method (logical shift right instead\nof arithmetic short right).  The compiler is free to chose the method,\nso it was only wrong in the sense that it didn't work as intended by us.\n\nReported-by: Øyvind A. Holm <sunny@sunbase.org>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nDoes this fix all the warnings, and do the tests pass?  I can only\nreproduce the \"shifting to far to the right\" warning..\n\n xdiff/xutils.c |   18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 78549e3..9504eae 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -280,19 +280,11 @@ static inline long count_masked_bytes(unsigned long mask)\n \t\tlong a = (REPEAT_BYTE(0x01) / 0xff + 1);\n \t\treturn mask * a >> (sizeof(long) * 7);\n \t} else {\n-\t\t/*\n-\t\t * Modified Carl Chatfield G+ version for 32-bit *\n-\t\t *\n-\t\t * (a) gives us\n-\t\t *   -1 (0, ff), 0 (ffff) or 1 (ffffff)\n-\t\t * (b) gives us\n-\t\t *   0 for 0, 1 for (ff ffff ffffff)\n-\t\t * (a+b+1) gives us\n-\t\t *   correct 0-3 bytemask count result\n-\t\t */\n-\t\tlong a = (mask - 256) >> 23;\n-\t\tlong b = mask & 1;\n-\t\treturn a + b + 1;\n+\t\t/* Carl Chatfield / Jan Achrenius G+ version for 32-bit */\n+\t\t/* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */\n+\t\tlong a = (0x0ff0001 + mask) >> 23;\n+\t\t/* Fix the 1 for 00 case */\n+\t\treturn a & mask;\n \t}\n }\n \n-- \n1.7.10.2\n"},{"id":"191972","messageId":"8762bn7101.fsf@thomas.inf.ethz.ch","threadId":"30543","inReplyTo":"4FBBF8D7.7050701@lsrfire.ath.cx","subject":"Re: [PATCH 2/3] xdiff: avoid more compiler warnings with XDL_FAST_HASH on 32-bit machines","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-05-23T08:30:38Z","receivedAt":"2012-05-23T08:30:38Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Hide literals that can cause compiler warnings for 32-bit architectures in\n> expressions that evaluate to small numbers there.  Some compilers warn that\n> 0x0001020304050608 won't fit into a 32-bit long, others that shifting right\n> by 56 bits clears a 32-bit value completely.\n>\n> The correct values are calculated in the 64-bit case, which is all that matters\n> in this if-branch.\n>\n> Reported-by: Øyvind A. Holm <sunny@sunbase.org>\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n\nThanks for fixing this.  As far as logic and review goes, both patches\n\nAcked-by: Thomas Rast <trast@student.ethz.ch>\n\nI haven't checked whether it actually fixes the warnings, however.\n\n> ---\n>  xdiff/xutils.c |    8 +++++++-\n>  1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n> index 8580da7..78549e3 100644\n> --- a/xdiff/xutils.c\n> +++ b/xdiff/xutils.c\n> @@ -272,7 +272,13 @@ static inline long count_masked_bytes(unsigned long mask)\n>  \t\t * that works for the bytemasks without having to\n>  \t\t * mask them first.\n>  \t\t */\n> -\t\treturn mask * 0x0001020304050608 >> 56;\n> +\t\t/*\n> +\t\t * return mask * 0x0001020304050608 >> 56;\n> +\t\t *\n> +\t\t * Doing it like this avoids warnings on 32-bit machines.\n> +\t\t */\n> +\t\tlong a = (REPEAT_BYTE(0x01) / 0xff + 1);\n> +\t\treturn mask * a >> (sizeof(long) * 7);\n>  \t} else {\n>  \t\t/*\n>  \t\t * Modified Carl Chatfield G+ version for 32-bit *\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"192163","messageId":"CAA787rkRZudv96wJ4WPFWt7k0Y628dvhxrPes1g6rbw8C_Kiog@mail.gmail.com","threadId":"30543","inReplyTo":"4FBBF8E9.7020103@lsrfire.ath.cx","subject":"Re: [PATCH 3/3] xdiff: import new 32-bit version of count_masked_bytes()","fromName":"Øyvind A. Holm","fromEmail":"sunny@sunbase.org","sentAt":"2012-05-25T15:18:00Z","receivedAt":"2012-05-25T15:18:00Z","isPatch":true,"sender":{"key":"sunny@sunbase.org","avatar":"https://avatars.githubusercontent.com/u/113445?v=4"},"body":"On 22 May 2012 22:36, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:\n> Import the latest 32-bit implementation of count_masked_bytes() from\n> Linux (arch/x86/include/asm/word-at-a-time.h).  It's shorter and avoids\n> overflows and negative numbers.\n>\n> This fixes test failures on 32-bit, where negative partial results had\n> been shifted right using the \"wrong\" method (logical shift right instead\n> of arithmetic short right).  The compiler is free to chose the method,\n> so it was only wrong in the sense that it didn't work as intended by us.\n>\n> Reported-by: Øyvind A. Holm <sunny@sunbase.org>\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n> Does this fix all the warnings, and do the tests pass?  I can only\n> reproduce the \"shifting to far to the right\" warning..\n\nHi, I finally got around to test this, and it compiles flawlessly with\nyour three patches. No warnings, and the test suite seems to run\nwithout errors.\n\nNice work, sir.\n\nCheers,\nØyvind\n"}]}