{"thread":{"id":"20651","subject":"[PATCH] block-sha1: Windows declares ntohl() in winsock2.h","startedAt":"2009-08-18T07:15:57Z","lastAt":"2009-08-20T02:45:40Z","messageCount":34,"participants":["Johannes Sixt","Sebastian Schuberth","Junio C Hamano","Artur Skawina","Linus Torvalds","Nicolas Pitre","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"121061","messageId":"4A8A552D.6020407@viscovery.net","threadId":"20651","inReplyTo":null,"subject":"[PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-08-18T07:15:57Z","receivedAt":"2009-08-18T07:15:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nThis is a minimal fix to compile block-sha1 on Windows. I did not do any\nbenchmarks whether the implementation of ntohl() is actually faster than\nbytewise access and shifts.\n\nSigned-off-by: Johannes Sixt \n---\n I would appreciate if our Windows experts could tell whether the\n implementation of ntohl/htonl is worth its money or whether we should\n go with the generic byte access plus shifts.\n\n the function call over\n block-sha1/sha1.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex a1228cf..67c1ee8 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -7,7 +7,11 @@\n */\n\n #include \n+#ifndef _WIN32\n #include \n+#else\n+#include \n+#endif\n\n #include \"sha1.h\"\n\n-- \n1.6.4.1179.g9a91.dirty\n"},{"id":"121078","messageId":"4A8A8661.5060908@gmail.com","threadId":"20651","inReplyTo":"4A8A552D.6020407@viscovery.net","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-08-18T10:45:53Z","receivedAt":"2009-08-18T10:45:53Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"> This is a minimal fix to compile block-sha1 on Windows. I did not do any\n> benchmarks whether the implementation of ntohl() is actually faster than\n> bytewise access and shifts.\n\nAs ntohl()/htonl() are function calls (that internally do shifts), I \ndoubt they're faster than the shift macros, though I haven't measured \nit. However, I do not suggest to go for the macros on Windows/Intel, but \nto apply the following patch on top of your patch:\n\n From 34402f0e5691ca46cd63c930eef8fc9cadf80493 Mon Sep 17 00:00:00 2001\nFrom: Sebastian Schuberth \nDate: Tue, 18 Aug 2009 12:33:35 +0200\nSubject: [PATCH] block-sha1: On Intel, use bswap built-in in favor of \nntohl()/htonl()\n\nOn Windows/Intel, ntohl()/htonl() are function calls that do shifts to \nswap the\nbyte order. Using the native bswap instruction boths gets rid of the \nshifts and\nthe function call overhead to gain some performance.\n\nSigned-off-by: Sebastian Schuberth \n---\n block-sha1/sha1.c | 15 ++++++++++-----\n 1 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex f2830c0..07f2937 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -66,15 +66,20 @@\n\n /*\n * Performance might be improved if the CPU architecture is OK with\n- * unaligned 32-bit loads and a fast ntohl() is available.\n+ * unaligned 32-bit loads and a fast ntohl() is available. On Intel,\n+ * use the bswap built-in to get rid of the function call overhead.\n * Otherwise fall back to byte loads and shifts which is portable,\n * and is faster on architectures with memory alignment issues.\n */\n\n-#if defined(__i386__) || defined(__x86_64__) || \\\n- defined(__ppc__) || defined(__ppc64__) || \\\n- defined(__powerpc__) || defined(__powerpc64__) || \\\n- defined(__s390__) || defined(__s390x__)\n+#if defined(__i386__) || defined(__x86_64__)\n+\n+#define get_be32(p)\t__builtin_bswap32(*(unsigned int *)(p))\n+#define put_be32(p, v)\tdo { *(unsigned int *)(p) = \n__builtin_bswap32(v); } while (0)\n+\n+#elif 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-- \n1.6.4.169.g64d5.dirty\n"},{"id":"121079","messageId":"7vtz05idn0.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"4A8A8661.5060908@gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T11:07:31Z","receivedAt":"2009-08-18T11:07:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> As ntohl()/htonl() are function calls (that internally do shifts), I\n> doubt they're faster than the shift macros, though I haven't measured\n> it. However, I do not suggest to go for the macros on Windows/Intel,\n> but to apply the following patch on top of your patch:\n\nYour proposed commit log message makes it sound as if this change is\nlimited to Windows, but it is not protected with \"#ifdef WIN32\"; the\nchange should be applicable to non-windows but the message is misleading.\n\nIt should help any i386/amd64 platform whose ntohl()/htonl() is crappy, as\nlong as __builtin_bswap32() is supported by the compiler. And it should\nnot harm other platforms, nor i386/amd64 whose ntohl()/htonl() are sane.\nAs i386/amd64 part of block-sha1/sha1.c has gcc dependency already, I\nthink it would be safe to assume __builtin_bswap32() is available.\n\nBut I'd want an Ack/Nack from the original authors (Cc'ed).\n\nIt seems that your patch is linewrapped, so please be careful _if_ it\nneeds to be modified and resent (if this version gets trivially acked I\ncan fix it up when applying and in such a case there is no need to\nresend).\n\n> From: Sebastian Schuberth \n> Date: Tue, 18 Aug 2009 12:33:35 +0200\n> Subject: [PATCH] block-sha1: On Intel, use bswap built-in in favor of\n> ntohl()/htonl()\n>\n> On Windows/Intel, ntohl()/htonl() are function calls that do shifts to\n> swap the\n> byte order. Using the native bswap instruction boths gets rid of the\n> shifts and\n> the function call overhead to gain some performance.\n>\n> Signed-off-by: Sebastian Schuberth \n> ---\n> block-sha1/sha1.c | 15 ++++++++++-----\n> 1 files changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\n> index f2830c0..07f2937 100644\n> --- a/block-sha1/sha1.c\n> +++ b/block-sha1/sha1.c\n> @@ -66,15 +66,20 @@\n>\n> /*\n> * Performance might be improved if the CPU architecture is OK with\n> - * unaligned 32-bit loads and a fast ntohl() is available.\n> + * unaligned 32-bit loads and a fast ntohl() is available. On Intel,\n> + * use the bswap built-in to get rid of the function call overhead.\n> * Otherwise fall back to byte loads and shifts which is portable,\n> * and is faster on architectures with memory alignment issues.\n> */\n>\n> -#if defined(__i386__) || defined(__x86_64__) || \\\n> - defined(__ppc__) || defined(__ppc64__) || \\\n> - defined(__powerpc__) || defined(__powerpc64__) || \\\n> - defined(__s390__) || defined(__s390x__)\n> +#if defined(__i386__) || defined(__x86_64__)\n> +\n> +#define get_be32(p)\t__builtin_bswap32(*(unsigned int *)(p))\n> +#define put_be32(p, v)\tdo { *(unsigned int *)(p) =\n> __builtin_bswap32(v); } while (0)\n> +\n> +#elif 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> -- \n> 1.6.4.169.g64d5.dirty\n"},{"id":"121081","messageId":"4A8A9044.9030405@gmail.com","threadId":"20651","inReplyTo":"7vtz05idn0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-08-18T11:28:04Z","receivedAt":"2009-08-18T11:28:04Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 18.08.2009 13:07, Junio C Hamano wrote:\n\n> Your proposed commit log message makes it sound as if this change is\n> limited to Windows, but it is not protected with \"#ifdef WIN32\"; the\n> change should be applicable to non-windows but the message is misleading.\n\nTrue, the change should apply for x86, no matter what the OS. I wanted to express that *on Windows* I know ntohl()/htonl() are function that do shifts internally. Maybe on some other OS these have better implementations (for x86).\n\n> It seems that your patch is linewrapped, so please be careful _if_ it\n> needs to be modified and resent (if this version gets trivially acked I\n> can fix it up when applying and in such a case there is no need to\n> resend).\n\nHere's an updated version of the patch that both improves the commit message and fixes the line wrap.\n\n From b4f40f73e8bf410c975b3f29d10ca779343e3095 Mon Sep 17 00:00:00 2001\nFrom: Sebastian Schuberth \nDate: Tue, 18 Aug 2009 12:33:35 +0200\nSubject: [PATCH] block-sha1: On x86, use bswap built-in in favor of ntohl()/htonl()\n\nOn x86 and compatible, use the native bswap instruction in favor of ntohl()/\nhtonl(). In the best case, this gets rid of function calls to crappy\nimplementations, in the worst case, it should be no slower than sane (inlined)\nimplementations. The current code depends on GCC already, so relying on\n__builtin_bswap32() to be available should be safe.\n\nSigned-off-by: Sebastian Schuberth \n---\n block-sha1/sha1.c | 15 ++++++++++-----\n 1 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex f2830c0..07f2937 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -66,15 +66,20 @@\n \n /*\n * Performance might be improved if the CPU architecture is OK with\n- * unaligned 32-bit loads and a fast ntohl() is available.\n+ * unaligned 32-bit loads and a fast ntohl() is available. On Intel,\n+ * use the bswap built-in to get rid of the function call overhead.\n * Otherwise fall back to byte loads and shifts which is portable,\n * and is faster on architectures with memory alignment issues.\n */\n \n-#if defined(__i386__) || defined(__x86_64__) || \\\n- defined(__ppc__) || defined(__ppc64__) || \\\n- defined(__powerpc__) || defined(__powerpc64__) || \\\n- defined(__s390__) || defined(__s390x__)\n+#if defined(__i386__) || defined(__x86_64__)\n+\n+#define get_be32(p)\t__builtin_bswap32(*(unsigned int *)(p))\n+#define put_be32(p, v)\tdo { *(unsigned int *)(p) = __builtin_bswap32(v); } while (0)\n+\n+#elif 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-- \n1.6.4.169.g64d5.dirty\n"},{"id":"121095","messageId":"4A8AA511.1060205@gmail.com","threadId":"20651","inReplyTo":"4A8A8661.5060908@gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Artur Skawina","fromEmail":"art.08.09@gmail.com","sentAt":"2009-08-18T12:56:49Z","receivedAt":"2009-08-18T12:56:49Z","isPatch":true,"sender":{"key":"art.08.09@gmail.com","avatar":null},"body":"Sebastian Schuberth wrote:\n> As ntohl()/htonl() are function calls (that internally do shifts), I\n> doubt they're faster than the shift macros, though I haven't measured\n> it. However, I do not suggest to go for the macros on Windows/Intel, but\n> to apply the following patch on top of your patch:\n\n> On Windows/Intel, ntohl()/htonl() are function calls that do shifts to\n> swap the\n> byte order. Using the native bswap instruction boths gets rid of the\n> shifts and\n> the function call overhead to gain some performance.\n\nUmm, nothing like this should be needed on linux; the compiler/glibc\nwill choose bswap itself. (see endian.h and bits/byteswap.h).\nI did try using __builtin_bswap32 directly and the result was a few\n(3 or 4, iirc) differently scheduled instructions, that's all, no\nperformance difference.\n\n> * Performance might be improved if the CPU architecture is OK with\n> - * unaligned 32-bit loads and a fast ntohl() is available.\n> + * unaligned 32-bit loads and a fast ntohl() is available. On Intel,\n> + * use the bswap built-in to get rid of the function call overhead.\n> * Otherwise fall back to byte loads and shifts which is portable,\n> * and is faster on architectures with memory alignment issues.\n> */\n> \n> -#if defined(__i386__) || defined(__x86_64__) || \\\n> - defined(__ppc__) || defined(__ppc64__) || \\\n> - defined(__powerpc__) || defined(__powerpc64__) || \\\n> - defined(__s390__) || defined(__s390x__)\n> +#if defined(__i386__) || defined(__x86_64__)\n>\n> +#define get_be32(p) __builtin_bswap32(*(unsigned int *)(p))\n> +#define put_be32(p, v) do { *(unsigned int *)(p) = __builtin_bswap32(v); } while (0)\n> +\n> +#elif defined(__ppc__) || defined(__ppc64__) || \\\n> + defined(__powerpc__) || defined(__powerpc64__) || \\\n> + defined(__s390__) || defined(__s390x__)\n\nI'd limit it to windows and any other ia32 platform that doesn't pick the\nbswaps itself; as is, it just adds an unnecessary hidden gcc dependency.\n\nHmm, it's actually a gcc-4.3+ dependency, so it won't even build w/ gcc 4.2;\nsomething like this would be required: \"(__GNUC__>=4 && __GNUC_MINOR__>=3)\" .\n\nartur\n"},{"id":"121103","messageId":"bdca99240908180617n75dfd0b5nfe069aba6e74b722@mail.gmail.com","threadId":"20651","inReplyTo":"4A8AA511.1060205@gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-08-18T13:17:33Z","receivedAt":"2009-08-18T13:17:33Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Aug 18, 2009 at 14:56, Artur Skawina wrote:\n\n> I did try using __builtin_bswap32 directly and the result was a few\n> (3 or 4, iirc) differently scheduled instructions, that's all, no\n> performance difference.\n\n[...]\n\n> I'd limit it to windows and any other ia32 platform that doesn't pick the\n> bswaps itself; as is, it just adds an unnecessary hidden gcc dependency.\n>\n> Hmm, it's actually a gcc-4.3+ dependency, so it won't even build w/ gcc 4.2;\n> something like this would be required: \"(__GNUC__>=4 && __GNUC_MINOR__>=3)\" .\n\nSo, as you say the code makes no difference under Linux, would you be\nOK with just testing for GCC 4.3+, and not for Windows? That would get\nrid of the \"hidden\" GCC dependency and not make the preprocessor\nchecks overly complex. Moreover, limiting my patch to any \"platform\nthat doesn't pick the bswaps itself\" could possibly require\nmaintenance on compiler / CRT updates.\n\n-- \nSebastian Schuberth\n"},{"id":"121104","messageId":"7v4os5gs0p.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"bdca99240908180617n75dfd0b5nfe069aba6e74b722@mail.gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T13:39:50Z","receivedAt":"2009-08-18T13:39:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Tue, Aug 18, 2009 at 14:56, Artur Skawina wrote:\n> ...\n>> I'd limit it to windows and any other ia32 platform that doesn't pick the\n>> bswaps itself; as is, it just adds an unnecessary hidden gcc dependency.\n>>\n>> Hmm, it's actually a gcc-4.3+ dependency, so it won't even build w/ gcc 4.2;\n>> something like this would be required: \"(__GNUC__>=4 && __GNUC_MINOR__>=3)\" .\n>\n> So, as you say the code makes no difference under Linux, would you be\n> OK with just testing for GCC 4.3+, and not for Windows? That would get\n> rid of the \"hidden\" GCC dependency and not make the preprocessor\n> checks overly complex. Moreover, limiting my patch to any \"platform\n> that doesn't pick the bswaps itself\" could possibly require\n> maintenance on compiler / CRT updates.\n\nI would say that should be fine, but I'd let Linus and Nico to overrule me\non this if they have any input.\n"},{"id":"121108","messageId":"alpine.LFD.2.01.0908180836440.3162@localhost.localdomain","threadId":"20651","inReplyTo":"7v4os5gs0p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-18T15:40:24Z","receivedAt":"2009-08-18T15:40:24Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Aug 2009, Junio C Hamano wrote:\n\n> Sebastian Schuberth writes:\n> \n> > On Tue, Aug 18, 2009 at 14:56, Artur Skawina wrote:\n> > ...\n> >> I'd limit it to windows and any other ia32 platform that doesn't pick the\n> >> bswaps itself; as is, it just adds an unnecessary hidden gcc dependency.\n> >>\n> >> Hmm, it's actually a gcc-4.3+ dependency, so it won't even build w/ gcc 4.2;\n> >> something like this would be required: \"(__GNUC__>=4 && __GNUC_MINOR__>=3)\" .\n> >\n> > So, as you say the code makes no difference under Linux, would you be\n> > OK with just testing for GCC 4.3+, and not for Windows? That would get\n> > rid of the \"hidden\" GCC dependency and not make the preprocessor\n> > checks overly complex. Moreover, limiting my patch to any \"platform\n> > that doesn't pick the bswaps itself\" could possibly require\n> > maintenance on compiler / CRT updates.\n> \n> I would say that should be fine, but I'd let Linus and Nico to overrule me\n> on this if they have any input.\n\nI'd suggest not using a gcc builtin, since if you're using gcc you might \nas well just use inline asm that has been around forever (unlike the \nbuiltin).\n\nJust do:\n\n\t#if defined(__GNUC__)\n\t#define htonl(x) ({ unsigned int __res; \\\n\t\t__asm__(\"bswap %0\":\"=r\" (__res):\"0\" (x)); \\\n\t\t__res; })\n\t#define ntohl(x) htonl(x)\n\t#endif\n\nor similar in the x86 section that does the rol/ror thing.\n\n\t\t\tLinus\n"},{"id":"121112","messageId":"alpine.LFD.2.01.0908180906441.3162@localhost.localdomain","threadId":"20651","inReplyTo":"alpine.LFD.2.01.0908180836440.3162@localhost.localdomain","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-18T16:08:38Z","receivedAt":"2009-08-18T16:08:38Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Aug 2009, Linus Torvalds wrote:\n> \n> I'd suggest not using a gcc builtin, since if you're using gcc you might \n> as well just use inline asm that has been around forever (unlike the \n> builtin).\n\nThat seems to be what glibc does too.\n\nHere's a patch.\n\n\t\tLinus\n---\n block-sha1/sha1.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 464cb25..e6e7170 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -22,6 +22,11 @@\n #define SHA_ROL(x,n)\tSHA_ASM(\"rol\", x, n)\n #define SHA_ROR(x,n)\tSHA_ASM(\"ror\", x, n)\n \n+#undef htonl\n+#undef ntohl\n+#define htonl(x) ({ unsigned int __res; __asm__(\"bswap %0\":\"=r\" (__res):\"0\" (x)); __res; })\n+#define ntohl(x) htonl(x)\n+\n #else\n \n #define SHA_ROT(X,l,r)\t(((X) << (l)) | ((X) >> (r)))\n"},{"id":"121115","messageId":"bdca99240908180923x49213f30q79cf9424c6aa8202@mail.gmail.com","threadId":"20651","inReplyTo":"alpine.LFD.2.01.0908180906441.3162@localhost.localdomain","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-08-18T16:23:58Z","receivedAt":"2009-08-18T16:23:58Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Aug 18, 2009 at 18:08, Linus\nTorvalds wrote:\n\n>> I'd suggest not using a gcc builtin, since if you're using gcc you might\n>> as well just use inline asm that has been around forever (unlike the\n>> builtin).\n>\n> That seems to be what glibc does too.\n>\n> Here's a patch.\n\nLooks good to me, compiles & runs fine on Windows (with Hannes' patch\nalso applied).\n\n-- \nSebastian Schuberth\n"},{"id":"121117","messageId":"alpine.LFD.2.00.0908181147510.6044@xanadu.home","threadId":"20651","inReplyTo":"7v4os5gs0p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T16:30:36Z","receivedAt":"2009-08-18T16:30:36Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Junio C Hamano wrote:\n\n> Sebastian Schuberth writes:\n> \n> > On Tue, Aug 18, 2009 at 14:56, Artur Skawina wrote:\n> > ...\n> >> I'd limit it to windows and any other ia32 platform that doesn't pick the\n> >> bswaps itself; as is, it just adds an unnecessary hidden gcc dependency.\n> >>\n> >> Hmm, it's actually a gcc-4.3+ dependency, so it won't even build w/ gcc 4.2;\n> >> something like this would be required: \"(__GNUC__>=4 && __GNUC_MINOR__>=3)\" .\n> >\n> > So, as you say the code makes no difference under Linux, would you be\n> > OK with just testing for GCC 4.3+, and not for Windows? That would get\n> > rid of the \"hidden\" GCC dependency and not make the preprocessor\n> > checks overly complex. Moreover, limiting my patch to any \"platform\n> > that doesn't pick the bswaps itself\" could possibly require\n> > maintenance on compiler / CRT updates.\n> \n> I would say that should be fine, but I'd let Linus and Nico to overrule me\n> on this if they have any input.\n\nWell... Given that git already uses ntohl/htonl quite extensively in \nits core already, I'd suggest making this more globally available \ninstead.\n\n\nNicolas\n"},{"id":"121118","messageId":"alpine.LFD.2.00.0908181240400.6044@xanadu.home","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181147510.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T16:43:08Z","receivedAt":"2009-08-18T16:43:08Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Nicolas Pitre wrote:\n\n> Well... Given that git already uses ntohl/htonl quite extensively in \n> its core already, I'd suggest making this more globally available \n> instead.\n\nWhat about something like this?\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 464cb25..51a27c1 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -4,9 +4,7 @@\n * and to avoid unnecessary copies into the context array.\n */\n \n-#include \n-#include \n-\n+#include \"../git-compat-util.h\"\n #include \"sha1.h\"\n \n #if defined(__i386__) || defined(__x86_64__)\ndiff --git a/compat/bswap.h b/compat/bswap.h\nnew file mode 100644\nindex 0000000..b436360\n--- /dev/null\n+++ b/compat/bswap.h\n@@ -0,0 +1,19 @@\n+/*\n+ * Let's make sure we always have a sane definition for ntohl()/htonl().\n+ * Some libraries define those as a function call, just to perform byte\n+ * shifting, bringing significant overhead to what should be a sinple\n+ * operation.\n+ */\n+\n+#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n+\n+#define bswap32(x) ({ \\\n+\tunsigned int __res; \\\n+\t__asm__(\"bswap %0\" : \"=r\" (__res) : \"0\" (x)); \\\n+\t__res; })\n+#undef ntohl\n+#undef htonl\n+#define ntohl(x) bswap32(x)\n+#define htonl(x) bswap32(x)\n+\n+#endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9f941e4..000859e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -176,6 +176,8 @@ extern char *gitbasename(char *);\n #endif\n #endif\n \n+#include \"compat/bswap.h\"\n+\n /* General helper functions */\n extern void usage(const char *err) NORETURN;\n extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));\n"},{"id":"121119","messageId":"7v63clf4xs.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"bdca99240908180923x49213f30q79cf9424c6aa8202@mail.gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T16:43:43Z","receivedAt":"2009-08-18T16:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Tue, Aug 18, 2009 at 18:08, Linus\n> Torvalds wrote:\n>\n>>> I'd suggest not using a gcc builtin, since if you're using gcc you might\n>>> as well just use inline asm that has been around forever (unlike the\n>>> builtin).\n>>\n>> That seems to be what glibc does too.\n>>\n>> Here's a patch.\n>\n> Looks good to me, compiles & runs fine on Windows (with Hannes' patch\n> also applied).\n\nBut the Windows part that avoids arpa/inet.h and includes winsock2.h, only\nto undef the two macros immediately after doing so, now looks quite silly.\nAre there non i386/amd64 Windows we care about?\n\nSquashing Linus's and Hannes's patch here is what I came up with.\n\n-- >8 --\nblock-sha1: avoid potentially inefficient ntohl/htonl on i386/x86-64\n\nJohannes Sixt reports that on Windows ntohl()/htonl() are not found in\n, and minimally we need to include instead.\nSebastian Schuberth points out that they are implemented as out-of-line\nfunctions on Windows, which defeats the use of these byteorder \"macros\"\nfor performance.\n\nUse bswap instruction through gcc inline asm instead on i386/x86-64\nas a generic solution to this.\n\nSigned-off-by: Johannes Sixt \nSigned-off-by: Linus Torvalds ,\n---\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex a1228cf..fa909a3 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -7,8 +7,6 @@\n */\n \n #include \n-#include \n-\n #include \"sha1.h\"\n \n #if defined(__i386__) || defined(__x86_64__)\n@@ -24,8 +22,15 @@\n #define SHA_ROL(x,n)\tSHA_ASM(\"rol\", x, n)\n #define SHA_ROR(x,n)\tSHA_ASM(\"ror\", x, n)\n \n+#undef htonl\n+#undef ntohl\n+#define htonl(x) ({ unsigned int __res; __asm__(\"bswap %0\":\"=r\" (__res):\"0\" (x)); __res; })\n+#define ntohl(x) htonl(x)\n+\n #else\n \n+#include \n+\n #define SHA_ROT(X,l,r)\t(((X) << (l)) | ((X) >> (r)))\n #define SHA_ROL(X,n)\tSHA_ROT(X,n,32-(n))\n #define SHA_ROR(X,n)\tSHA_ROT(X,32-(n),n)\n"},{"id":"121122","messageId":"7v1vn9f4mz.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181240400.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T16:50:12Z","receivedAt":"2009-08-18T16:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 18 Aug 2009, Nicolas Pitre wrote:\n>\n>> Well... Given that git already uses ntohl/htonl quite extensively in \n>> its core already, I'd suggest making this more globally available \n>> instead.\n>\n> What about something like this?\n\nFor git's own use, I would be much happier with this change.\n\nBut given that there are some people wanting to snarf block-sha1/*.[ch]\nout to use them standalone, I have a slight hesitation against introducing\nthe dependency to git-compat-util.h, making it unclear to them that all\nthis file wants from outside are ntohl, htonl and memcpy.\n"},{"id":"121126","messageId":"bdca99240908180959h69f37671k4d526fbf4814e8d1@mail.gmail.com","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181240400.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2009-08-18T16:59:18Z","receivedAt":"2009-08-18T16:59:18Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Aug 18, 2009 at 18:43, Nicolas Pitre wrote:\n\n>> Well...  Given that git already uses ntohl/htonl quite extensively in\n>> its core already, I'd suggest making this more globally available\n>> instead.\n>\n> What about something like this?\n\nI like the idea of making bswap available more globally, but I'm not\nsure if it's worth to introduce a new file for only that purpose.\nIsn't there already a central header for such things?\n\nMoreover, including compat/bswap.h would only give you ntohl()/htonl()\non one platform. For consistency, I'd expect to get those for any\nplatform if I include compat/bswap.h, but maybe I'm not aware of some\nGit source code rules.\n\nFinally, there's a typo in your comment saying \"sinple\" instead of \"simple\".\n\n-- \nSebastian Schuberth\n"},{"id":"121128","messageId":"7vpratdpc8.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"bdca99240908180959h69f37671k4d526fbf4814e8d1@mail.gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T17:05:59Z","receivedAt":"2009-08-18T17:05:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> I like the idea of making bswap available more globally, but I'm not\n> sure if it's worth to introduce a new file for only that purpose.\n> Isn't there already a central header for such things?\n>\n> Moreover, including compat/bswap.h would only give you ntohl()/htonl()\n> on one platform.\n\nWe do not include compat/ directly from the source; git-compat-util.h is\nsupposed to be the first thing included (as some platforms have peculiar\nrequirements on the order in which system header files are included, and\none of the reasons git-compat-util.h is there). Hence by including it,\nyou get ntohl/htonl everywhere.\n\nTo reduce confusion, you may want to rename compat/bswap.h to something\nlike compat/ntohl-htonl-fix.h ;-)\n"},{"id":"121138","messageId":"alpine.LFD.2.00.0908181357330.6044@xanadu.home","threadId":"20651","inReplyTo":"7v1vn9f4mz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T18:01:57Z","receivedAt":"2009-08-18T18:01:57Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > On Tue, 18 Aug 2009, Nicolas Pitre wrote:\n> >\n> >> Well... Given that git already uses ntohl/htonl quite extensively in \n> >> its core already, I'd suggest making this more globally available \n> >> instead.\n> >\n> > What about something like this?\n> \n> For git's own use, I would be much happier with this change.\n> \n> But given that there are some people wanting to snarf block-sha1/*.[ch]\n> out to use them standalone, I have a slight hesitation against introducing\n> the dependency to git-compat-util.h, making it unclear to them that all\n> this file wants from outside are ntohl, htonl and memcpy.\n\nShould we really care to keep our code suboptimal just to make it \nreadily reusable by other projects? That seems a bit backward to me.\n\nWe might simply add a comment telling people what git-compat-util.h is \nactually for, namely memcpy(), ntohl() and htonl(). That should be \ntrivial for them to use the appropriate substitute.\n\n\nNicolas\n"},{"id":"121140","messageId":"alpine.LFD.2.00.0908181403190.6044@xanadu.home","threadId":"20651","inReplyTo":"bdca99240908180959h69f37671k4d526fbf4814e8d1@mail.gmail.com","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T18:10:58Z","receivedAt":"2009-08-18T18:10:58Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Sebastian Schuberth wrote:\n\n> On Tue, Aug 18, 2009 at 18:43, Nicolas Pitre wrote:\n> \n> >> Well...  Given that git already uses ntohl/htonl quite extensively in\n> >> its core already, I'd suggest making this more globally available\n> >> instead.\n> >\n> > What about something like this?\n> \n> I like the idea of making bswap available more globally, but I'm not\n> sure if it's worth to introduce a new file for only that purpose.\n> Isn't there already a central header for such things?\n\nThat central header is already quite crowded. A bit of isolation might \nnot hurt.\n\nFurthermore, other platforms might wish to add their own (re)definitions \nfor those byte swap operations, so it has the potential to grow. I for \nexample have a better implementation for ARM than what is provided by \nglibc. (Yeah yeah, maybe glibc should be fixed instead, but that \nreasoning goes for all those other libraries too).\n\n> Moreover, including compat/bswap.h would only give you ntohl()/htonl()\n> on one platform. For consistency, I'd expect to get those for any\n> platform if I include compat/bswap.h, but maybe I'm not aware of some\n> Git source code rules.\n\nYou get it by default for all platforms already by including \ngit-compat-util.h. The compat/bswap.h is not meant to be included by \nrandom c files. If compat/bswap.h happens to contain a better version \nfor your architecture then it'll override the default one.\n\n> Finally, there's a typo in your comment saying \"sinple\" instead of \"simple\".\n\nThanks\n\n\nNicolas\n"},{"id":"121141","messageId":"alpine.LFD.2.00.0908181411320.6044@xanadu.home","threadId":"20651","inReplyTo":"7vpratdpc8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T18:16:35Z","receivedAt":"2009-08-18T18:16:35Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Junio C Hamano wrote:\n\n> To reduce confusion, you may want to rename compat/bswap.h to something\n> like compat/ntohl-htonl-fix.h ;-)\n\nBah. If you wish, you can edit the patch directly for this, unless you \nreally prefer me to repost. Maybe we might want to add a 8-byte \nversions of those as well eventually, which is why I chose a more \ngeneric name.\n\n\nNicolas\n"},{"id":"121144","messageId":"7vk511dk11.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181357330.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T19:00:42Z","receivedAt":"2009-08-18T19:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 18 Aug 2009, Junio C Hamano wrote:\n>\n>> For git's own use, I would be much happier with this change.\n>> \n>> But given that there are some people wanting to snarf block-sha1/*.[ch]\n>> out to use them standalone, I have a slight hesitation against introducing\n>> the dependency to git-compat-util.h, making it unclear to them that all\n>> this file wants from outside are ntohl, htonl and memcpy.\n>\n> Should we really care to keep our code suboptimal just to make it \n> readily reusable by other projects? That seems a bit backward to me.\n\nYou are right; and I should give a bit more credit to their intelligence.\nThe source (block-sha1/sha1.c) is short enough that they can figure this\nout for themselves even without any additional comments.\n\nAnother issue, especially with your \"openssl sha1 removal\" patch, is if we\ncan assume gcc everywhere. As far as I can tell, block-sha1/sha1.c will\nbe the first unconditional use of inline asm or statement expression on\ni386/amd64. Are folks on Solaris and other platforms Ok with this?\n"},{"id":"121145","messageId":"alpine.LFD.2.00.0908181516510.6044@xanadu.home","threadId":"20651","inReplyTo":"7vk511dk11.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T19:22:39Z","receivedAt":"2009-08-18T19:22:39Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > On Tue, 18 Aug 2009, Junio C Hamano wrote:\n> >\n> >> For git's own use, I would be much happier with this change.\n> >> \n> >> But given that there are some people wanting to snarf block-sha1/*.[ch]\n> >> out to use them standalone, I have a slight hesitation against introducing\n> >> the dependency to git-compat-util.h, making it unclear to them that all\n> >> this file wants from outside are ntohl, htonl and memcpy.\n> >\n> > Should we really care to keep our code suboptimal just to make it \n> > readily reusable by other projects? That seems a bit backward to me.\n> \n> You are right; and I should give a bit more credit to their intelligence.\n> The source (block-sha1/sha1.c) is short enough that they can figure this\n> out for themselves even without any additional comments.\n\nWell, I gave in and added a comment to the patch anyway, with more \nimprovements in the case of constant values. Patch follows.\n\n> Another issue, especially with your \"openssl sha1 removal\" patch, is if we\n> can assume gcc everywhere. As far as I can tell, block-sha1/sha1.c will\n> be the first unconditional use of inline asm or statement expression on\n> i386/amd64. Are folks on Solaris and other platforms Ok with this?\n\nI guess we can guard the first with \nifdef(__GNUC__) which should help \npeople with MSVC. That should take care of x86 at least.\n\nNicolas\n"},{"id":"121147","messageId":"alpine.LFD.2.00.0908181523430.6044@xanadu.home","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181516510.6044@xanadu.home","subject":"[PATCH] make sure byte swapping is optimal for git","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T19:26:55Z","receivedAt":"2009-08-18T19:26:55Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"We rely on ntohl() and htonl() to perform byte swapping in many places.\nHowever, some platforms have libraries providing really poor\nimplementations of those which might cause significant performance\nissues, especially with the block-sha1 code.\n\nSigned-off-by: Nicolas Pitre \n---\n\nOn Tue, 18 Aug 2009, Nicolas Pitre wrote:\n\n> Well, I gave in and added a comment to the patch anyway, with more \n> improvements in the case of constant values. Patch follows.\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 464cb25..d31f2e3 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -4,8 +4,8 @@\n * and to avoid unnecessary copies into the context array.\n */\n \n-#include \n-#include \n+/* this is only to get definitions for memcpy(), ntohl() and htonl() */\n+#include \"../git-compat-util.h\"\n \n #include \"sha1.h\"\n \ndiff --git a/compat/bswap.h b/compat/bswap.h\nnew file mode 100644\nindex 0000000..7246a12\n--- /dev/null\n+++ b/compat/bswap.h\n@@ -0,0 +1,36 @@\n+/*\n+ * Let's make sure we always have a sane definition for ntohl()/htonl().\n+ * Some libraries define those as a function call, just to perform byte\n+ * shifting, bringing significant overhead to what should be a simple\n+ * operation.\n+ */\n+\n+/*\n+ * Default version that the compiler ought to optimize properly with\n+ * constant values.\n+ */\n+static inline unsigned int default_swab32(unsigned int val)\n+{\n+\treturn (((val & 0xff000000) >> 24) |\n+\t\t((val & 0x00ff0000) >> 8) |\n+\t\t((val & 0x0000ff00) << 8) |\n+\t\t((val & 0x000000ff) << 24));\n+}\n+\n+#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n+\n+#define bswap32(x) ({ \\\n+\tunsigned int __res; \\\n+\tif (__builtin_constant_p(x)) { \\\n+\t\t__res = default_swab32(x); \\\n+\t} else { \\\n+\t\t__asm__(\"bswap %0\" : \"=r\" (__res) : \"0\" (x)); \\\n+\t} \\\n+\t__res; })\n+\n+#undef ntohl\n+#undef htonl\n+#define ntohl(x) bswap32(x)\n+#define htonl(x) bswap32(x)\n+\n+#endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9f941e4..000859e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -176,6 +176,8 @@ extern char *gitbasename(char *);\n #endif\n #endif\n \n+#include \"compat/bswap.h\"\n+\n /* General helper functions */\n extern void usage(const char *err) NORETURN;\n extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));\n"},{"id":"121148","messageId":"alpine.LFD.2.00.0908181534130.6044@xanadu.home","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181516510.6044@xanadu.home","subject":"[PATCH] block-sha1: guard gcc extensions with __GNUC__","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T19:37:22Z","receivedAt":"2009-08-18T19:37:22Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"With this, the code should now be portable to any C compiler.\n\nSigned-off-by: Nicolas Pitre \n---\n\nOn Tue, 18 Aug 2009, Nicolas Pitre wrote:\n\n> On Tue, 18 Aug 2009, Junio C Hamano wrote:\n> \n> > Another issue, especially with your \"openssl sha1 removal\" patch, is if we\n> > can assume gcc everywhere. As far as I can tell, block-sha1/sha1.c will\n> > be the first unconditional use of inline asm or statement expression on\n> > i386/amd64. Are folks on Solaris and other platforms Ok with this?\n> \n> I guess we can guard the first with ifdef(__GNUC__) which should help \n> people with MSVC. That should take care of x86 at least.\n\nHere it is.\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex d31f2e3..92d9121 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -9,7 +9,7 @@\n \n #include \"sha1.h\"\n \n-#if defined(__i386__) || defined(__x86_64__)\n+#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n \n /*\n * Force usage of rol or ror by selecting the one with the smaller constant.\n@@ -54,7 +54,7 @@\n \n #if defined(__i386__) || defined(__x86_64__)\n #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))\n-#elif defined(__arm__)\n+#elif defined(__GNUC__) && defined(__arm__)\n #define setW(x, val) do { W(x) = (val); __asm__(\"\":::\"memory\"); } while (0)\n #else\n #define setW(x, val) (W(x) = (val))\n"},{"id":"121149","messageId":"7vfxbodi6c.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"7vk511dk11.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T19:40:43Z","receivedAt":"2009-08-18T19:40:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Another issue, especially with your \"openssl sha1 removal\" patch, is if we\n> can assume gcc everywhere. As far as I can tell, block-sha1/sha1.c will\n> be the first unconditional use of inline asm or statement expression on\n> i386/amd64. Are folks on Solaris and other platforms Ok with this?\n\nAhhh, your bswap.h is a no-op unless you use gcc, so my worry was\nunfounded.\n"},{"id":"121151","messageId":"XJM0H8pTiCJpryS-arPltHCHwsm0djqVixaH1NwBqT2pci2MA9karw@cipher.nrlssc.navy.mil","threadId":"20651","inReplyTo":"7vk511dk11.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2009-08-18T19:56:56Z","receivedAt":"2009-08-18T19:56:56Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"Junio C Hamano wrote:\n\n> Another issue, especially with your \"openssl sha1 removal\" patch, is if we\n> can assume gcc everywhere. As far as I can tell, block-sha1/sha1.c will\n> be the first unconditional use of inline asm or statement expression on\n> i386/amd64. Are folks on Solaris and other platforms Ok with this?\n\nThe SUNWspro compiler doesn't set __i386__. Instead it sets __i386, and\nI think __x86_64 and __amd64 where appropriate. So, compilation with\nthe SUNWspro compiler on x86 is currently unaffected by these changes and\nfalls back to the generic routines.\n\nIt seems that v5.10 of the compiler can grok both the __asm__ statements\nand the ({...}) naked block notation and passes all of the tests when the\nblock_sha1 code is modified to add defined(__i386) to each of the macro\nstatements.\n\nThe 5.8 version cannot grok the naked block, and requires spelling __asm__\nas __asm for inline assembly. Even then it appears that there is a bug in\nthe assembly that is produced (a google search told me so), so the assembly\ncode does not successfully compile.\n\nI haven't had much time to think about how or whether to address this.\n\nAdding something like the following would get ugly real quick:\n\n (defined(__i386) && defined(__SUNPRO_C) && (__SUNPRO_C >= 0x5100))\n\nFor now, the code compiles fine using the SUNWspro compiler on x86 even if\nit is suboptimal compared to gcc. It is still an improvement over the\nmozilla code.\n\n-brandon\n"},{"id":"121153","messageId":"alpine.LFD.2.00.0908181607240.6044@xanadu.home","threadId":"20651","inReplyTo":"XJM0H8pTiCJpryS-arPltHCHwsm0djqVixaH1NwBqT2pci2MA9karw@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T20:10:37Z","receivedAt":"2009-08-18T20:10:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Brandon Casey wrote:\n\n> The SUNWspro compiler doesn't set __i386__. Instead it sets __i386, and\n> I think __x86_64 and __amd64 where appropriate. So, compilation with\n> the SUNWspro compiler on x86 is currently unaffected by these changes and\n> falls back to the generic routines.\n> \n> It seems that v5.10 of the compiler can grok both the __asm__ statements\n> and the ({...}) naked block notation and passes all of the tests when the\n> block_sha1 code is modified to add defined(__i386) to each of the macro\n> statements.\n\nDoes it really implement the gcc inline assembly syntax?\n\n\nNicolas\n"},{"id":"121154","messageId":"7vprasc1vu.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181411320.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T20:17:57Z","receivedAt":"2009-08-18T20:17:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 18 Aug 2009, Junio C Hamano wrote:\n>\n>> To reduce confusion, you may want to rename compat/bswap.h to something\n>> like compat/ntohl-htonl-fix.h ;-)\n>\n> Bah. If you wish, you can edit the patch directly for this, unless you \n> really prefer me to repost. Maybe we might want to add a 8-byte \n> versions of those as well eventually, which is why I chose a more \n> generic name.\n\nOk, here is what I came up with after many squashing...\n\n-- >8 --\nFrom: Nicolas Pitre \nDate: Tue, 18 Aug 2009 12:43:08 -0400\nSubject: [PATCH] bswap: avoid potentially inefficient ntohl/htonl on i386/x86-64\n\nJohannes Sixt reports that on Windows ntohl()/htonl() are not found in\n, and as a minimal fix we need to include \ninstead. Sebastian Schuberth points out that they are implemented as\nout-of-line functions on Windows, which defeats these byteorder \"macros\"\nused for performance.\n\nUse bswap instruction through gcc inline asm instead on i386/x86-64\nas a generic solution to this.\n\nSigned-off-by: Johannes Sixt \nSigned-off-by: Linus Torvalds \nSigned-off-by: Nicolas Pitre \nSigned-off-by: Junio C Hamano \n---\n block-sha1/sha1.c | 4 +---\n compat/bswap.h | 19 +++++++++++++++++++\n git-compat-util.h | 2 ++\n 3 files changed, 22 insertions(+), 3 deletions(-)\n create mode 100644 compat/bswap.h\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 464cb25..51a27c1 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -4,9 +4,7 @@\n * and to avoid unnecessary copies into the context array.\n */\n \n-#include \n-#include \n-\n+#include \"../git-compat-util.h\"\n #include \"sha1.h\"\n \n #if defined(__i386__) || defined(__x86_64__)\ndiff --git a/compat/bswap.h b/compat/bswap.h\nnew file mode 100644\nindex 0000000..78fd2df\n--- /dev/null\n+++ b/compat/bswap.h\n@@ -0,0 +1,19 @@\n+/*\n+ * Let's make sure we always have a sane definition for ntohl()/htonl().\n+ * Some libraries define those as a function call, just to perform byte\n+ * swapping, bringing significant overhead to what should be a simple\n+ * operation.\n+ */\n+\n+#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n+\n+#define bswap32(x) ({ \\\n+\tunsigned int __res; \\\n+\t__asm__(\"bswap %0\" : \"=r\" (__res) : \"0\" (x)); \\\n+\t__res; })\n+#undef ntohl\n+#undef htonl\n+#define ntohl(x) bswap32(x)\n+#define htonl(x) bswap32(x)\n+\n+#endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9f941e4..000859e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -176,6 +176,8 @@ extern char *gitbasename(char *);\n #endif\n #endif\n \n+#include \"compat/bswap.h\"\n+\n /* General helper functions */\n extern void usage(const char *err) NORETURN;\n extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));\n-- \n1.6.4.245.g50659\n"},{"id":"121157","messageId":"alpine.LFD.2.01.0908181316330.3158@localhost.localdomain","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181607240.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-18T20:25:32Z","receivedAt":"2009-08-18T20:25:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Aug 2009, Nicolas Pitre wrote:\n> \n> Does it really implement the gcc inline assembly syntax?\n\nA fair number of compilers do. The Intel compiler does too.\n\nThe reason? The gcc inline asm is as close to a standard there is, and is \nfairly expressive without being the mess that is MSC. So if you do inline \nasm at all (and considering the target market for a C compiler, most do), \ngcc asm is a good thing to aim for.\n\n\t\t\tLinus\n"},{"id":"121158","messageId":"08Paa_FqYXS4u78q8DsW-e7xpWkmcOdrkJUSHb8zhns8DwkPS-Rpxw@cipher.nrlssc.navy.mil","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908181607240.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2009-08-18T20:29:35Z","receivedAt":"2009-08-18T20:29:35Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"Nicolas Pitre wrote:\n> On Tue, 18 Aug 2009, Brandon Casey wrote:\n> \n>> The SUNWspro compiler doesn't set __i386__. Instead it sets __i386, and\n>> I think __x86_64 and __amd64 where appropriate. So, compilation with\n>> the SUNWspro compiler on x86 is currently unaffected by these changes and\n>> falls back to the generic routines.\n>>\n>> It seems that v5.10 of the compiler can grok both the __asm__ statements\n>> and the ({...}) naked block notation and passes all of the tests when the\n>> block_sha1 code is modified to add defined(__i386) to each of the macro\n>> statements.\n> \n> Does it really implement the gcc inline assembly syntax?\n\nYes, I think it does. I actually extracted the assembly from sha1.c and tested\nin a separate test program. The v5.10 sunwspro compiler compiles and executes\nit correctly.\n\n-brandon\n"},{"id":"121159","messageId":"7v7hx0c1at.fsf@alter.siamese.dyndns.org","threadId":"20651","inReplyTo":"7vprasc1vu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T20:30:34Z","receivedAt":"2009-08-18T20:30:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nicolas Pitre writes:\n>\n>> On Tue, 18 Aug 2009, Junio C Hamano wrote:\n>>\n>>> To reduce confusion, you may want to rename compat/bswap.h to something\n>>> like compat/ntohl-htonl-fix.h ;-)\n>>\n>> Bah. If you wish, you can edit the patch directly for this, unless you \n>> really prefer me to repost. Maybe we might want to add a 8-byte \n>> versions of those as well eventually, which is why I chose a more \n>> generic name.\n>\n> Ok, here is what I came up with after many squashing...\n\nMeh, our mails crossed. I'll chuck this one and use your\n\n [PATCH] make sure byte swapping is optimal for git\n\npatch. Do you want default_swab32 be mmoved inside the\n\n #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n\nblock?\n"},{"id":"121161","messageId":"alpine.LFD.2.00.0908181646241.6044@xanadu.home","threadId":"20651","inReplyTo":"7v7hx0c1at.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-18T20:49:43Z","receivedAt":"2009-08-18T20:49:43Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Junio C Hamano wrote:\n\n> Meh, our mails crossed. I'll chuck this one and use your\n> \n> [PATCH] make sure byte swapping is optimal for git\n> \n> patch. Do you want default_swab32 be mmoved inside the\n> \n> #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n> \n> block?\n\nNot necessarily. It is generic code that other compilers/architectures \nmight use as well.\n\n\nNicolas\n"},{"id":"121309","messageId":"alpine.LFD.2.00.0908192201180.6044@xanadu.home","threadId":"20651","inReplyTo":"XJM0H8pTiCJpryS-arPltHCHwsm0djqVixaH1NwBqT2pci2MA9karw@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-20T02:26:37Z","receivedAt":"2009-08-20T02:26:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Aug 2009, Brandon Casey wrote:\n\n> The SUNWspro compiler doesn't set __i386__. Instead it sets __i386, and\n> I think __x86_64 and __amd64 where appropriate. So, compilation with\n> the SUNWspro compiler on x86 is currently unaffected by these changes and\n> falls back to the generic routines.\n> \n> It seems that v5.10 of the compiler can grok both the __asm__ statements\n> and the ({...}) naked block notation and passes all of the tests when the\n> block_sha1 code is modified to add defined(__i386) to each of the macro\n> statements.\n> \n> The 5.8 version cannot grok the naked block, and requires spelling __asm__\n> as __asm for inline assembly. Even then it appears that there is a bug in\n> the assembly that is produced (a google search told me so), so the assembly\n> code does not successfully compile.\n> \n> I haven't had much time to think about how or whether to address this.\n> \n> Adding something like the following would get ugly real quick:\n> \n> (defined(__i386) && defined(__SUNPRO_C) && (__SUNPRO_C >= 0x5100))\n\nI think the best solution in this case might simply be to add something \nlike this somewhere at the top of git-compat-util.h after the system \nincludes:\n\n/*\n * The SUNWspro compiler uses different symbols than gcc.\n * Let's standardize on the gcc flavor.\n */\n#if defined(__i386) && !defined(__i386__)\n#define __i386__\n#endif\n#if (defined(__x86_64) || defined(__amd64)) && !defined(__x86_64__)\n#define __x86_64__\n#endif\n/*\n * SUNWspro from version 5.10 supports gcc extensions such as gcc's \n * statement expressions and extended inline asm, so let's pretend...\n */\n#if defined(__SUNPRO_C) && (__SUNPRO_C >= 0x5100))\n#define __GNUC__\n#endif\n\n\nNicolas\n"},{"id":"121310","messageId":"alpine.LFD.2.01.0908191929350.3158@localhost.localdomain","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908192201180.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-08-20T02:30:33Z","receivedAt":"2009-08-20T02:30:33Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 19 Aug 2009, Nicolas Pitre wrote:\n> \n> I think the best solution in this case might simply be to add something \n> like this somewhere at the top of git-compat-util.h after the system \n> includes:\n\nI think we can just test __i386.\n\nGcc defines that one too (in fact, in general, gcc always defines both the \n__x and the __x__ versions, although I'm sure there are exceptions)\n\n\t\tLinus\n"},{"id":"121312","messageId":"swB1Tb3ZWzkSggFnb4OlG99G0HKv1Bv9CEnk5hC0dseH5Wq60whE0w@cipher.nrlssc.navy.mil","threadId":"20651","inReplyTo":"alpine.LFD.2.00.0908192201180.6044@xanadu.home","subject":"Re: [PATCH] block-sha1: Windows declares ntohl() in winsock2.h","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2009-08-20T02:45:40Z","receivedAt":"2009-08-20T02:45:40Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"Nicolas Pitre wrote:\n> On Tue, 18 Aug 2009, Brandon Casey wrote:\n> \n>> The SUNWspro compiler doesn't set __i386__. Instead it sets __i386, and\n>> I think __x86_64 and __amd64 where appropriate. So, compilation with\n>> the SUNWspro compiler on x86 is currently unaffected by these changes and\n>> falls back to the generic routines.\n>>\n>> It seems that v5.10 of the compiler can grok both the __asm__ statements\n>> and the ({...}) naked block notation and passes all of the tests when the\n>> block_sha1 code is modified to add defined(__i386) to each of the macro\n>> statements.\n>>\n>> The 5.8 version cannot grok the naked block, and requires spelling __asm__\n>> as __asm for inline assembly. Even then it appears that there is a bug in\n>> the assembly that is produced (a google search told me so), so the assembly\n>> code does not successfully compile.\n>>\n>> I haven't had much time to think about how or whether to address this.\n>>\n>> Adding something like the following would get ugly real quick:\n>>\n>> (defined(__i386) && defined(__SUNPRO_C) && (__SUNPRO_C >= 0x5100))\n> \n> I think the best solution in this case might simply be to add something \n> like this somewhere at the top of git-compat-util.h after the system \n> includes:\n> \n> /*\n> * The SUNWspro compiler uses different symbols than gcc.\n> * Let's standardize on the gcc flavor.\n> */\n> #if defined(__i386) && !defined(__i386__)\n> #define __i386__\n> #endif\n> #if (defined(__x86_64) || defined(__amd64)) && !defined(__x86_64__)\n> #define __x86_64__\n> #endif\n\nYes, I had this idea too.\n\n> /*\n> * SUNWspro from version 5.10 supports gcc extensions such as gcc's \n> * statement expressions and extended inline asm, so let's pretend...\n> */\n> #if defined(__SUNPRO_C) && (__SUNPRO_C >= 0x5100))\n> #define __GNUC__\n> #endif\n\nI hadn't thought of this. I'll test to make sure the other statements\nthat are protected by ifdef __GNUC__ work correctly with SUNWspro v5.10.\n\n-brandon\n"}]}