{"thread":{"id":"20568","subject":"[PATCH] block-sha1: more good unaligned memory access candidates","startedAt":"2009-08-13T04:29:14Z","lastAt":"2009-08-15T18:43:11Z","messageCount":15,"participants":["Nicolas Pitre","Linus Torvalds","Junio C Hamano","Brandon Casey","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"120480","messageId":"alpine.LFD.2.00.0908130017080.10633@xanadu.home","threadId":"20568","inReplyTo":null,"subject":"[PATCH] block-sha1: more good unaligned memory access candidates","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-13T04:29:14Z","receivedAt":"2009-08-13T04:29:14Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"In addition to X86, PowerPC and S390 are capable of unaligned memory \naccesses.\n\nSigned-off-by: Nicolas Pitre \n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex d3121f7..e5a1007 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -67,7 +67,10 @@\n * and is faster on architectures with memory alignment issues.\n */\n \n-#if defined(__i386__) || defined(__x86_64__)\n+#if defined(__i386__) || defined(__x86_64__) || \\\n+ defined(__ppc__) || defined(__ppc64__) || \\\n+ defined(__powerpc__) || defined(__powerpc64__) || \\\n+ defined(__s390__) || defined(__s390x__)\n \n #define get_be32(p)\tntohl(*(unsigned int *)(p))\n #define put_be32(p, v)\tdo { *(unsigned int *)(p) = htonl(v); } while (0)\n"},{"id":"120540","messageId":"alpine.LFD.2.01.0908130934400.28882@localhost.localdomain","threadId":"20568","inReplyTo":"alpine.LFD.2.00.0908130017080.10633@xanadu.home","subject":"Re: [PATCH] block-sha1: more good unaligned memory access candidates","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-13T16:45:54Z","receivedAt":"2009-08-13T16:45:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Aug 2009, Nicolas Pitre wrote:\n>\n> In addition to X86, PowerPC and S390 are capable of unaligned memory \n> accesses.\n> \n> Signed-off-by: Nicolas Pitre \n\nAck on all your patches (1-3 + this). Looks fine to me.\n\nI do wonder if we should try to do basically \"per-architecture hack \nheader-files\", and then have for each architecture a small trivial \n'hack-x86.h' kind of thing that just does\n\n\t/* x86 hacks */\n\t#define get_be32(p)\tntohl(*(unsigned int *)(p))\n\t#define put_be32(p, v)\tdo { *(unsigned int *)(p) = htonl(v); } while (0)\n\t#define setW(x, val)\t(*(volatile unsigned int *)&W(x) = (val))\n\n\t#define SHA_ASM(op, x, n) ({ unsigned int __res; __asm__(op \" %1,%0\":\"=r\" (__res):\"i\" (n), \"0\" (x)); __res; })\n\t#define SHA_ROL(x,n) SHA_ASM(\"rol\", x, n)\n\t#define SHA_ROR(x,n) SHA_ASM(\"ror\", x, n)\n\nand then we'd have each architecture separated out. Add a few \n\"generic helpers\":\n\n - be32-generic.h:\n\n\t#define get_be32(p) ( \\\n\t\t(*((unsigned char *)(p) + 0) << 24) | \\\n\t\t(*((unsigned char *)(p) + 1) << 16) | \\\n\t\t(*((unsigned char *)(p) + 2) << 8) | \\\n\t\t(*((unsigned char *)(p) + 3) << 0) )\n\n\t#define put_be32(p, v) do { \\\n\t\tunsigned int __v = (v); \\\n\t\t*((unsigned char *)(p) + 0) = __v >> 24; \\\n\t\t*((unsigned char *)(p) + 1) = __v >> 16; \\\n\t\t*((unsigned char *)(p) + 2) = __v >> 8; \\\n\t\t*((unsigned char *)(p) + 3) = __v >> 0; } while (0)\n\n - rotate-generic.h:\n\n\t#define SHA_ROT(X,l,r) (((X) << (l)) | ((X) >> (r)))\n\t#define SHA_ROL(X,n) SHA_ROT(X,n,32-(n))\n\t#define SHA_ROR(X,n) SHA_ROT(X,32-(n),n)\n\nthat architectures could use when they want to use some particular\nportable version. Then, add a final \"hack-generic.h\" with the fallback\ncases that just does\n\n\t#include \"be32-generic.h\"\n\t#include \"rotate-generic.h\"\n\t#define setW(x,val) (W(x) = (val))\n\nand you'd have all the hacks separated out and fairly easily used by\ndifferent architectures.. (ie the ARM version would just look like\n\n - hack-arm.h:\n\n\t#include \"be32-generic.h\" \n\t#include \"rotate-generic.h\"\n\t#define setW(x, val) do { W(x) = (val); __asm__(\"\":::\"memory\"); } while (0)\n\nand you'd be all done.\n\nHmm? I don't know if this kind of generalization is strictly needed, so \nI'm just throwing it out as an idea.\n\n\t\tLinus\n"},{"id":"120549","messageId":"alpine.LFD.2.00.0908131304520.10633@xanadu.home","threadId":"20568","inReplyTo":"alpine.LFD.2.01.0908130934400.28882@localhost.localdomain","subject":"Re: [PATCH] block-sha1: more good unaligned memory access candidates","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-13T17:23:58Z","receivedAt":"2009-08-13T17:23:58Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 13 Aug 2009, Linus Torvalds wrote:\n\n> \n> \n> On Thu, 13 Aug 2009, Nicolas Pitre wrote:\n> >\n> > In addition to X86, PowerPC and S390 are capable of unaligned memory \n> > accesses.\n> > \n> > Signed-off-by: Nicolas Pitre \n> \n> Ack on all your patches (1-3 + this). Looks fine to me.\n> \n> I do wonder if we should try to do basically \"per-architecture hack \n> header-files\", and then have for each architecture a small trivial \n> 'hack-x86.h' kind of thing that just does\n> \n> \t/* x86 hacks */\n> \t#define get_be32(p)\tntohl(*(unsigned int *)(p))\n> \t#define put_be32(p, v)\tdo { *(unsigned int *)(p) = htonl(v); } while (0)\n> \t#define setW(x, val)\t(*(volatile unsigned int *)&W(x) = (val))\n> \n> \t#define SHA_ASM(op, x, n) ({ unsigned int __res; __asm__(op \" %1,%0\":\"=r\" (__res):\"i\" (n), \"0\" (x)); __res; })\n> \t#define SHA_ROL(x,n) SHA_ASM(\"rol\", x, n)\n> \t#define SHA_ROR(x,n) SHA_ASM(\"ror\", x, n)\n> \n> and then we'd have each architecture separated out. Add a few \n> \"generic helpers\":\n> \n> - be32-generic.h:\n> \n> \t#define get_be32(p) ( \\\n> \t\t(*((unsigned char *)(p) + 0) << 24) | \\\n> \t\t(*((unsigned char *)(p) + 1) << 16) | \\\n> \t\t(*((unsigned char *)(p) + 2) << 8) | \\\n> \t\t(*((unsigned char *)(p) + 3) << 0) )\n> \n> \t#define put_be32(p, v) do { \\\n> \t\tunsigned int __v = (v); \\\n> \t\t*((unsigned char *)(p) + 0) = __v >> 24; \\\n> \t\t*((unsigned char *)(p) + 1) = __v >> 16; \\\n> \t\t*((unsigned char *)(p) + 2) = __v >> 8; \\\n> \t\t*((unsigned char *)(p) + 3) = __v >> 0; } while (0)\n> \n> - rotate-generic.h:\n> \n> \t#define SHA_ROT(X,l,r) (((X) << (l)) | ((X) >> (r)))\n> \t#define SHA_ROL(X,n) SHA_ROT(X,n,32-(n))\n> \t#define SHA_ROR(X,n) SHA_ROT(X,32-(n),n)\n> \n> that architectures could use when they want to use some particular\n> portable version. Then, add a final \"hack-generic.h\" with the fallback\n> cases that just does\n> \n> \t#include \"be32-generic.h\"\n> \t#include \"rotate-generic.h\"\n> \t#define setW(x,val) (W(x) = (val))\n> \n> and you'd have all the hacks separated out and fairly easily used by\n> different architectures.. (ie the ARM version would just look like\n> \n> - hack-arm.h:\n> \n> \t#include \"be32-generic.h\" \n> \t#include \"rotate-generic.h\"\n> \t#define setW(x, val) do { W(x) = (val); __asm__(\"\":::\"memory\"); } while (0)\n> \n> and you'd be all done.\n> \n> Hmm? I don't know if this kind of generalization is strictly needed, so \n> I'm just throwing it out as an idea.\n\nWell... first I don't think there are much else we can do with that \ncode, i.e. there is probably not so many more hacks to add if any. With \nmy last patch I consider this ready for general consumption.\n\nAnd given that the only user is this very sha1 implementation then there \nis not much value in abstracting them in header files.\n\nAnd the point of patch 2/3 was to move hack variants for the same \npurpose together making it easier to document and understand (and \npossibly modify) them. Your suggestion would go in the opposite \ndirection entirely.\n\nAs it is now, I was about to suggest:\n\n\tgit mv block-sha1/sha1.[ch] .\n\trmdir block-sha1\n\trm -r mozilla-sha1\n\trm -r arm\n\trm -r ppc \n\nand remove support for openssl's SHA1 usage, making this implementation \nunconditional. After all it is faster, or so close to be faster than \nthe alternatives, that we should probably cut on the extra dependency \nand simplify portability issues at the same time.\n\n\nNicolas\n"},{"id":"120566","messageId":"7v63crbja2.fsf@alter.siamese.dyndns.org","threadId":"20568","inReplyTo":"alpine.LFD.2.00.0908131304520.10633@xanadu.home","subject":"Re: [PATCH] block-sha1: more good unaligned memory access candidates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-13T19:33:57Z","receivedAt":"2009-08-13T19:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> As it is now, I was about to suggest:\n>\n> \tgit mv block-sha1/sha1.[ch] .\n> \trmdir block-sha1\n> \trm -r mozilla-sha1\n> \trm -r arm\n> \trm -r ppc \n>\n> and remove support for openssl's SHA1 usage, making this implementation \n> unconditional. After all it is faster, or so close to be faster than \n> the alternatives, that we should probably cut on the extra dependency \n> and simplify portability issues at the same time.\n\nWow. Is it now faster than the arm/ and ppc/ hand-tweaked assembly?\n"},{"id":"120569","messageId":"alpine.LFD.2.01.0908131246300.28882@localhost.localdomain","threadId":"20568","inReplyTo":"7v63crbja2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: more good unaligned memory access candidates","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-13T19:54:06Z","receivedAt":"2009-08-13T19:54:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Aug 2009, Junio C Hamano wrote:\n> \n> Wow. Is it now faster than the arm/ and ppc/ hand-tweaked assembly?\n\nFor the good cases, yes.\n\nFor POWER, with gcc-4.4, the C code apparently outperforms the asm code on \nPOWER6. The asm code is scheduled for POWER4, and I think outperforms the \nC code there. Also, when compiling in 64-bit mode (with \"-m64\"), at least \nsome versions of gcc seem to do some stupid things and add extra zero \nextension stuff, and that performed suboptimally at least on a PPC G5.\n\nSo it's certainly not a clear case of \"the C code outperforms the asm \ncode\", but in BenH's tests, the best numbers really did come from the C \nversion. With some silly cases of at least some versions gcc screwing up \n(not reload, but zero extension), and making it noticeably slower.\n\nIOW, the PPC situation really isn't that different from x86. \n\n\t\t\tLinus\n"},{"id":"120573","messageId":"alpine.LFD.2.00.0908131603240.10633@xanadu.home","threadId":"20568","inReplyTo":"7v63crbja2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: more good unaligned memory access candidates","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-13T20:13:16Z","receivedAt":"2009-08-13T20:13:16Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 13 Aug 2009, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > As it is now, I was about to suggest:\n> >\n> > \tgit mv block-sha1/sha1.[ch] .\n> > \trmdir block-sha1\n> > \trm -r mozilla-sha1\n> > \trm -r arm\n> > \trm -r ppc \n> >\n> > and remove support for openssl's SHA1 usage, making this implementation \n> > unconditional. After all it is faster, or so close to be faster than \n> > the alternatives, that we should probably cut on the extra dependency \n> > and simplify portability issues at the same time.\n> \n> Wow. Is it now faster than the arm/ and ppc/ hand-tweaked assembly?\n\nIt is indeed faster than the ARM assembly version by far, and faster \nthan all the alternative implementations too, but with a 7x increase in \ncompiled code size. In the context of Git I think this is a good \ncompromize. Making the assembly version faster than the C version could \nbe possible, but that would require quite some work and I don't expect \nthe gain to be significant, certainly not worth the trouble.\n\nFurthermore the C version can be used to generate ARM Thumb code while \nthe asm version cannot without yet more work.\n\n\nNicolas\n"},{"id":"120672","messageId":"_-0l9qI_nKm-VVtKlL5hzen4bhcyRmiZ1z4jSmKK1LwECXueziRDzSTyacEehOkoqsfYIaEUlds@cipher.nrlssc.navy.mil","threadId":"20568","inReplyTo":"alpine.LFD.2.00.0908130017080.10633@xanadu.home","subject":"[PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-08-14T22:52:15Z","receivedAt":"2009-08-14T22:52:15Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey \n\nSome compilers produce errors when arithmetic is attempted on pointers to\nvoid. So cast to uintptr_t when performing arithmetic on void pointers.\n\nSigned-off-by: Brandon Casey \n---\n\n\nThis, on top of Nico's last patch \"block-sha1: more good unaligned memory\naccess candidates\", allows this series to compile and run successfully on\nSun Sparc and IRIX MIPS for me.\n\nIt produces no differences on Linux using gcc. gcc 3.4.6 and 4.1.2 produce\nan identical binary for me on x86 and x86-64 with and without this change.\nAny conceivable negative effects on other arch's?\n\n-brandon\n\n\n block-sha1/sha1.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex e5a1007..ccaba9e 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -246,14 +246,14 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)\n \t\tmemcpy(lenW + (char *)ctx->W, data, left);\n \t\tlenW = (lenW + left) & 63;\n \t\tlen -= left;\n-\t\tdata += left;\n+\t\tdata = (const void*) ((uintptr_t) data + left);\n \t\tif (lenW)\n \t\t\treturn;\n \t\tblk_SHA1_Block(ctx, ctx->W);\n \t}\n \twhile (len >= 64) {\n \t\tblk_SHA1_Block(ctx, data);\n-\t\tdata += 64;\n+\t\tdata = (const void*) ((uintptr_t) data + 64);\n \t\tlen -= 64;\n \t}\n \tif (len)\n-- \n1.6.4\n"},{"id":"120675","messageId":"alpine.DEB.1.00.0908150207270.8306@pacific.mpi-cbg.de","threadId":"20568","inReplyTo":"_-0l9qI_nKm-VVtKlL5hzen4bhcyRmiZ1z4jSmKK1LwECXueziRDzSTyacEehOkoqsfYIaEUlds@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-08-15T00:08:45Z","receivedAt":"2009-08-15T00:08:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 14 Aug 2009, Brandon Casey wrote:\n\n> Some compilers produce errors when arithmetic is attempted on pointers to\n> void. So cast to uintptr_t when performing arithmetic on void pointers.\n\nI am confused. Is sizeof(*(uintptr_t)NULL) not larger than 1, and as a \nconsequence ((uintptr_t)p)+1 not different from ((void *)p)+1?\n\nCiao,\nDscho\n"},{"id":"120676","messageId":"alpine.LFD.2.01.0908141714040.3162@localhost.localdomain","threadId":"20568","inReplyTo":"alpine.DEB.1.00.0908150207270.8306@pacific.mpi-cbg.de","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-15T00:19:32Z","receivedAt":"2009-08-15T00:19:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 15 Aug 2009, Johannes Schindelin wrote:\n> \n> I am confused. Is sizeof(*(uintptr_t)NULL) not larger than 1\n\nYou can't do that. \"uintptr_t\" isn't actually a pointer. It's just a \nunsigned integer value large enough to contain a pointer.\n\nSo it's _not_ a \"pointer to uint\". It's \"uint that can contain all the \nbits of a poitner\".\n\n> and as a consequence ((uintptr_t)p)+1 not different from ((void *)p)+1?\n\nNo, they're the same.\n\nThat said, I suspect it might as well be cast to \"const char *\", and then \nhopefully you only need one cast.\n\nSo maybe it could be written as\n\n\tdata = (const char *) data + len;\n\ninstead, and avoid the second cast (because the assignment should be ok, \nsince it's assigning back to a \"const void *\").\n\n\t\tLinus\n"},{"id":"120677","messageId":"5HEgWEs64y4Ty7FnekstRP8Q71pNi_FTweLbMhYWc6zoZBA4jkJqpQ@cipher.nrlssc.navy.mil","threadId":"20568","inReplyTo":"alpine.LFD.2.01.0908141714040.3162@localhost.localdomain","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-08-15T00:44:38Z","receivedAt":"2009-08-15T00:44:38Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Linus Torvalds wrote:\n> I suspect it might as well be cast to \"const char *\", and then \n> hopefully you only need one cast.\n> \n> So maybe it could be written as\n> \n> \tdata = (const char *) data + len;\n> \n> instead, and avoid the second cast (because the assignment should be ok, \n> since it's assigning back to a \"const void *\").\n\nYep, that's enough. It produces an identical binary on all platforms\nexcept Solaris x86 using SUNWspro compiler.\n\n-brandon\n"},{"id":"120678","messageId":"qZXfoQ5Gp5jKnRfKM6ee07YVX-hyS2HsJmQwAYdPVHw92cOuUcufJA@cipher.nrlssc.navy.mil","threadId":"20568","inReplyTo":"alpine.DEB.1.00.0908150207270.8306@pacific.mpi-cbg.de","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-08-15T00:52:46Z","receivedAt":"2009-08-15T00:52:46Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Johannes Schindelin wrote:\n> Hi,\n> \n> On Fri, 14 Aug 2009, Brandon Casey wrote:\n> \n>> Some compilers produce errors when arithmetic is attempted on pointers to\n>> void. So cast to uintptr_t when performing arithmetic on void pointers.\n> \n> I am confused. Is sizeof(*(uintptr_t)NULL) not larger than 1, and as a \n> consequence ((uintptr_t)p)+1 not different from ((void *)p)+1?\n\nIf you try this:\n\n printf(\"NULL + 1: %u\\n\", (void*)NULL + 1);\n\nthe MIPSpro compiler complains like:\n\n The expression must be a pointer to a complete object type.\n\n printf(\"NULL + 1: %u\\n\", (void*)NULL + 1);\n ^\n\n 1 error detected in the compilation of \"test.c\".\n\n\nCompilers other than gcc at least issue a warning, if they do\nnot also fail.\n\n-brandon\n"},{"id":"120680","messageId":"7v8whl7ss9.fsf@alter.siamese.dyndns.org","threadId":"20568","inReplyTo":"5HEgWEs64y4Ty7FnekstRP8Q71pNi_FTweLbMhYWc6zoZBA4jkJqpQ@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-15T01:46:46Z","receivedAt":"2009-08-15T01:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Linus Torvalds wrote:\n>> I suspect it might as well be cast to \"const char *\", and then \n>> hopefully you only need one cast.\n>> \n>> So maybe it could be written as\n>> \n>> \tdata = (const char *) data + len;\n>> \n>> instead, and avoid the second cast (because the assignment should be ok, \n>> since it's assigning back to a \"const void *\").\n>\n> Yep, that's enough. It produces an identical binary on all platforms\n> except Solaris x86 using SUNWspro compiler.\n\nCasting to \"const char *\" to tell compilers that we are interested in byte\naddress differences/increments makes sense to me, but I do not know how to\ninterpret your last comment.\n\nDo you mean that SUNWspro compiler misbehave with the \"cast to char *\" and\ndo you meed your \"casting to uintptr_t to explicitly compute byte\naddresses as int\" to make it behave?\n"},{"id":"120685","messageId":"MecVLMUB15cEJQiZpwSBg9ysnmFSB1QYZfIQuSOStnFx2KoeScNXGQ@cipher.nrlssc.navy.mil","threadId":"20568","inReplyTo":"7v8whl7ss9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-08-15T02:34:10Z","receivedAt":"2009-08-15T02:34:10Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> Linus Torvalds wrote:\n>>> I suspect it might as well be cast to \"const char *\", and then \n>>> hopefully you only need one cast.\n>>>\n>>> So maybe it could be written as\n>>>\n>>> \tdata = (const char *) data + len;\n>>>\n>>> instead, and avoid the second cast (because the assignment should be ok, \n>>> since it's assigning back to a \"const void *\").\n>> Yep, that's enough. It produces an identical binary on all platforms\n>> except Solaris x86 using SUNWspro compiler.\n> \n> Casting to \"const char *\" to tell compilers that we are interested in byte\n> address differences/increments makes sense to me,\n\nYes, I agree.\n\n> but I do not know how to interpret your last comment.\n> Do you mean that SUNWspro compiler misbehave with the \"cast to char *\" and\n> do you meed your \"casting to uintptr_t to explicitly compute byte\n> addresses as int\" to make it behave?\n\nNo, sorry, the SUNWspro compiler produces a working binary with any of the\nthree versions: the original, cast to uintptr_t, or cast to const char*.\nThough it emits a warning with the original.\n\nI was just pointing out that the SUNWspro compiler does not produce an\nidentical binary on x86 for each of the three versions like the GNU\ncompiler does.\n\nSorry for the confusion. Linus's suggestion of casting to \"const char*\"\nshould be adopted.\n\n-brandon\n"},{"id":"120686","messageId":"7v63cp22cc.fsf@alter.siamese.dyndns.org","threadId":"20568","inReplyTo":"MecVLMUB15cEJQiZpwSBg9ysnmFSB1QYZfIQuSOStnFx2KoeScNXGQ@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-15T03:16:51Z","receivedAt":"2009-08-15T03:16:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> I was just pointing out that the SUNWspro compiler does not produce an\n> identical binary on x86 for each of the three versions like the GNU\n> compiler does.\n>\n> Sorry for the confusion. Linus's suggestion of casting to \"const char*\"\n> should be adopted.\n\nAh, I see. Thanks.\n\nLet's queue this, then.\n\n-- >8 --\nFrom: Brandon Casey \nDate: Fri, 14 Aug 2009 17:52:15 -0500\nSubject: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void * to char *\n\nSome compilers produce errors when arithmetic is attempted on pointers to\nvoid. We want computations done on byte addresses, so cast them to char *\nto work them around.\n\nSigned-off-by: Brandon Casey \nSigned-off-by: Junio C Hamano \n---\n block-sha1/sha1.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex e5a1007..464cb25 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -246,14 +246,14 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)\n \t\tmemcpy(lenW + (char *)ctx->W, data, left);\n \t\tlenW = (lenW + left) & 63;\n \t\tlen -= left;\n-\t\tdata += left;\n+\t\tdata = ((const char *)data + left);\n \t\tif (lenW)\n \t\t\treturn;\n \t\tblk_SHA1_Block(ctx, ctx->W);\n \t}\n \twhile (len >= 64) {\n \t\tblk_SHA1_Block(ctx, data);\n-\t\tdata += 64;\n+\t\tdata = ((const char *)data + 64);\n \t\tlen -= 64;\n \t}\n \tif (len)\n"},{"id":"120724","messageId":"alpine.LFD.2.01.0908151142470.3162@localhost.localdomain","threadId":"20568","inReplyTo":"7v63cp22cc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void* to uintptr_t","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-15T18:43:11Z","receivedAt":"2009-08-15T18:43:11Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 14 Aug 2009, Junio C Hamano wrote:\n>\n> From: Brandon Casey \n> Date: Fri, 14 Aug 2009 17:52:15 -0500\n> Subject: [PATCH] block-sha1/sha1.c: silence compiler complaints by casting void * to char *\n> \n> Some compilers produce errors when arithmetic is attempted on pointers to\n> void. We want computations done on byte addresses, so cast them to char *\n> to work them around.\n> \n> Signed-off-by: Brandon Casey \n> Signed-off-by: Junio C Hamano \n\nUnnecessary outer parentheses, but ack anyway.\n\n\t\tLinus\n"}]}