{"thread":{"id":"57517","subject":"[PATCH] block-sha1: remove use of assembly","startedAt":"2022-03-07T23:27:10Z","lastAt":"2022-03-10T19:18:43Z","messageCount":12,"participants":["brian m. carlson","Taylor Blau","Ævar Arnfjörð Bjarmason","Carlo Marcelo Arenas Belón","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"450672","messageId":"20220307232552.2799122-1-sandals@crustytoothpaste.net","threadId":"57517","inReplyTo":null,"subject":"[PATCH] block-sha1: remove use of assembly","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-07T23:25:52Z","receivedAt":"2022-03-07T23:27:10Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In the block SHA-1 code, we have special assembly code for i386 and\namd64 to perform rotations with assembly.  This is supposed to help pick\nthe correct rotation operation depending on which rotation is smaller,\nwhich can help some systems perform slightly better, since any circular\nrotation can be specified as either a rotate left or a rotate right.\nHowever, this isn't needed, so we should remove it.\n\nFirst, SHA-1, like SHA-2, uses fixed constant rotates.  Thus, all\nrotation amounts are known at compile time and are in fact baked into\nthe code.  Fortunately, peephole optimizers recognize rotations\nspecified in the normal way and automatically emit the correct code,\nincluding a preference for choosing a rotate left versus a rotate right.\nThis has been the case for well over a decade, and is a standard example\nof the utility of a peephole optimizer.\n\nMoreover, all modern CPUs, with the exception of extremely limited\nembedded CPUs such as some Cortex-M processors, provide a barrel\nshifter, which lets the CPU perform rotates of any bit amount in\nconstant time.  This is valuable for many cryptographic algorithms to\nimprove performance, and is required to prevent timing attacks in\nalgorithms which use data-dependent rotations (which don't include the\nhash algorithms we use).  As a result, even though the compiler does the\ncorrect optimization, it isn't even needed here and either a left or a\nright rotate is equally acceptable.\n\nIn fact, the SHA-256 code already takes this into account and just\nwrites the simple code using an inline function to let the compiler\noptimize it for us.\n\nThe downside of using this code, however, is that it uses a GCC\nextension, which makes the compiler complain when using -pedantic unless\nit's prefixed with __extension__.  We could fix that, but since it's\nnot needed, let's just remove it.  We haven't noticed this because\nalmost everyone uses the SHA1DC code instead, but it still shows up for\nsome people.\n---\nNormally here, I would say, just use the SHA1DC code.  However, at\nGitHub, this is wired up to a special piece of code in our Git\nimplementation that doesn't process attacker controlled data but needs\nto persist its internal hash state (to avoid needing to rehash a\npotentially large file, a log of operations performed, to which data is\nonly appended).\n\nThus, I'm sending in a fix here to avoid the warning, even though it's\nmy ultimate intention to remove it in favor of the SHA1DC code in the\nfuture.\n\n block-sha1/sha1.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 1bb6e7c069..5974cd7dd3 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -11,27 +11,10 @@\n \n #include \"sha1.h\"\n \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- * It _can_ generate slightly smaller code (a constant of 1 is special), but\n- * perhaps more importantly it's possibly faster on any uarch that does a\n- * rotate with a loop.\n- */\n-\n-#define SHA_ASM(op, x, n) ({ unsigned int __res; __asm__(op \" %1,%0\":\"=r\" (__res):\"i\" (n), \"0\" (x)); __res; })\n-#define SHA_ROL(x,n)\tSHA_ASM(\"rol\", x, n)\n-#define SHA_ROR(x,n)\tSHA_ASM(\"ror\", x, n)\n-\n-#else\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 \n-#endif\n-\n /*\n  * If you have 32 registers or more, the compiler can (and should)\n  * try to change the array[] accesses into registers. However, on\n"},{"id":"450674","messageId":"YiaWGrBNuk1+j89z@nand.local","threadId":"57517","inReplyTo":"20220307232552.2799122-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] block-sha1: remove use of assembly","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-07T23:32:42Z","receivedAt":"2022-03-07T23:32:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 07, 2022 at 11:25:52PM +0000, brian m. carlson wrote:\n> In the block SHA-1 code, we have special assembly code for i386 and\n> amd64 to perform rotations with assembly.  This is supposed to help pick\n> the correct rotation operation depending on which rotation is smaller,\n> which can help some systems perform slightly better, since any circular\n> rotation can be specified as either a rotate left or a rotate right.\n> However, this isn't needed, so we should remove it.\n\nAt -O1 or higher (at least on GCC) this optimization is indeed\nperformed. Here's a Godbolt example that shows this:\n\n    https://godbolt.org/z/9zMP93hr1\n\nso I agree that this code isn't helping us at all. And in the\nmeantime...\n\n> The downside of using this code, however, is that it uses a GCC\n> extension, which makes the compiler complain when using -pedantic unless\n> it's prefixed with __extension__.  We could fix that, but since it's\n> not needed, let's just remove it.  We haven't noticed this because\n> almost everyone uses the SHA1DC code instead, but it still shows up for\n> some people.\n\n...it makes it impossible to compile git if you have\n`BLK_SHA1=YesPlease` and `DEVELOPER=1` in your environment. So I am\nhappy to see this go.\n\nOn another note: missing Signed-off-by?\n\nThanks,\nTaylor\n"},{"id":"450698","messageId":"Yia9dQ32uYCCpIsm@camp.crustytoothpaste.net","threadId":"57517","inReplyTo":"YiaWGrBNuk1+j89z@nand.local","subject":"Re: [PATCH] block-sha1: remove use of assembly","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-08T02:20:37Z","receivedAt":"2022-03-08T02:20:42Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-03-07 at 23:32:42, Taylor Blau wrote:\n> On Mon, Mar 07, 2022 at 11:25:52PM +0000, brian m. carlson wrote:\n> > In the block SHA-1 code, we have special assembly code for i386 and\n> > amd64 to perform rotations with assembly.  This is supposed to help pick\n> > the correct rotation operation depending on which rotation is smaller,\n> > which can help some systems perform slightly better, since any circular\n> > rotation can be specified as either a rotate left or a rotate right.\n> > However, this isn't needed, so we should remove it.\n> \n> At -O1 or higher (at least on GCC) this optimization is indeed\n> performed. Here's a Godbolt example that shows this:\n> \n>     https://godbolt.org/z/9zMP93hr1\n> \n> so I agree that this code isn't helping us at all. And in the\n> meantime...\n\nThanks for providing a link.  I also did a similar test there with\nslightly different code (and unfortunately closed the window before\nsaving the link) but it demonstrated the same thing: that the compiler\ncan optimize this case adequately.  My (substantially) past experience\nwith testing GCC in this case has shown the same thing.\n\n> > The downside of using this code, however, is that it uses a GCC\n> > extension, which makes the compiler complain when using -pedantic unless\n> > it's prefixed with __extension__.  We could fix that, but since it's\n> > not needed, let's just remove it.  We haven't noticed this because\n> > almost everyone uses the SHA1DC code instead, but it still shows up for\n> > some people.\n> \n> ...it makes it impossible to compile git if you have\n> `BLK_SHA1=YesPlease` and `DEVELOPER=1` in your environment. So I am\n> happy to see this go.\n> \n> On another note: missing Signed-off-by?\n\nI'll send an otherwise unchanged v2 with that in a second.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"450699","messageId":"20220308022240.2809483-1-sandals@crustytoothpaste.net","threadId":"57517","inReplyTo":"20220307232552.2799122-1-sandals@crustytoothpaste.net","subject":"[PATCH v2] block-sha1: remove use of assembly","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-08T02:22:40Z","receivedAt":"2022-03-08T02:22:48Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In the block SHA-1 code, we have special assembly code for i386 and\namd64 to perform rotations with assembly.  This is supposed to help pick\nthe correct rotation operation depending on which rotation is smaller,\nwhich can help some systems perform slightly better, since any circular\nrotation can be specified as either a rotate left or a rotate right.\nHowever, this isn't needed, so we should remove it.\n\nFirst, SHA-1, like SHA-2, uses fixed constant rotates.  Thus, all\nrotation amounts are known at compile time and are in fact baked into\nthe code.  Fortunately, peephole optimizers recognize rotations\nspecified in the normal way and automatically emit the correct code,\nincluding a preference for choosing a rotate left versus a rotate right.\nThis has been the case for well over a decade, and is a standard example\nof the utility of a peephole optimizer.\n\nMoreover, all modern CPUs, with the exception of extremely limited\nembedded CPUs such as some Cortex-M processors, provide a barrel\nshifter, which lets the CPU perform rotates of any bit amount in\nconstant time.  This is valuable for many cryptographic algorithms to\nimprove performance, and is required to prevent timing attacks in\nalgorithms which use data-dependent rotations (which don't include the\nhash algorithms we use).  As a result, even though the compiler does the\ncorrect optimization, it isn't even needed here and either a left or a\nright rotate is equally acceptable.\n\nIn fact, the SHA-256 code already takes this into account and just\nwrites the simple code using an inline function to let the compiler\noptimize it for us.\n\nThe downside of using this code, however, is that it uses a GCC\nextension, which makes the compiler complain when using -pedantic unless\nit's prefixed with __extension__.  We could fix that, but since it's\nnot needed, let's just remove it.  We haven't noticed this because\nalmost everyone uses the SHA1DC code instead, but it still shows up for\nsome people.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nChanges from v1:\n* Add sign-off.\n\n block-sha1/sha1.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 1bb6e7c069..5974cd7dd3 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -11,27 +11,10 @@\n \n #include \"sha1.h\"\n \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- * It _can_ generate slightly smaller code (a constant of 1 is special), but\n- * perhaps more importantly it's possibly faster on any uarch that does a\n- * rotate with a loop.\n- */\n-\n-#define SHA_ASM(op, x, n) ({ unsigned int __res; __asm__(op \" %1,%0\":\"=r\" (__res):\"i\" (n), \"0\" (x)); __res; })\n-#define SHA_ROL(x,n)\tSHA_ASM(\"rol\", x, n)\n-#define SHA_ROR(x,n)\tSHA_ASM(\"ror\", x, n)\n-\n-#else\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 \n-#endif\n-\n /*\n  * If you have 32 registers or more, the compiler can (and should)\n  * try to change the array[] accesses into registers. However, on\n"},{"id":"450729","messageId":"220308.864k48y35f.gmgdl@evledraar.gmail.com","threadId":"57517","inReplyTo":"20220308022240.2809483-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] block-sha1: remove use of assembly","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-08T13:38:06Z","receivedAt":"2022-03-08T13:39:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 08 2022, brian m. carlson wrote:\n\nI think the $subject of the patch needs updating. It's not removing all\nthe assemply from the file, after this patch we still have the\nARM-specific assembly.\n\nI don't have a box to test that on, but I wonder if that also triggers\nthe pedantic mode?\n\nPerhaps:\n\n    block-sha1: remove superfluous i386 and x86-64 assembly\n\n?\n"},{"id":"450942","messageId":"Yikl2eGbc8sPsy5G@camp.crustytoothpaste.net","threadId":"57517","inReplyTo":"220308.864k48y35f.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] block-sha1: remove use of assembly","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-09T22:10:33Z","receivedAt":"2022-03-09T22:10:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-03-08 at 13:38:06, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Mar 08 2022, brian m. carlson wrote:\n> \n> I think the $subject of the patch needs updating. It's not removing all\n> the assemply from the file, after this patch we still have the\n> ARM-specific assembly.\n> \n> I don't have a box to test that on, but I wonder if that also triggers\n> the pedantic mode?\n> \n> Perhaps:\n> \n>     block-sha1: remove superfluous i386 and x86-64 assembly\n\nI suspect it has the same problem.  My inclination is to just remove it,\nbecause my guess is that the compiler has gotten smarter between 2009\nand now.\n\nI honestly intend to just remove this code in a future version because\neveryone not using SHA1DC has a security problem and we shouldn't offer\ninsecure options.\n\nHowever, I think for now, I'm just going to reroll this with the new\ntitle and then I can remove it in a future version unless somebody with\nan ARM system can relatively quickly tell me whether it's necessary.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"450945","messageId":"Yikq7POhuxeN1UPQ@nand.local","threadId":"57517","inReplyTo":"Yikl2eGbc8sPsy5G@camp.crustytoothpaste.net","subject":"Re: [PATCH v2] block-sha1: remove use of assembly","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-09T22:32:12Z","receivedAt":"2022-03-09T22:32:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 09, 2022 at 10:10:33PM +0000, brian m. carlson wrote:\n> On 2022-03-08 at 13:38:06, Ævar Arnfjörð Bjarmason wrote:\n> >\n> > On Tue, Mar 08 2022, brian m. carlson wrote:\n> >\n> > I think the $subject of the patch needs updating. It's not removing all\n> > the assemply from the file, after this patch we still have the\n> > ARM-specific assembly.\n> >\n> > I don't have a box to test that on, but I wonder if that also triggers\n> > the pedantic mode?\n> >\n> > Perhaps:\n> >\n> >     block-sha1: remove superfluous i386 and x86-64 assembly\n>\n> I suspect it has the same problem.  My inclination is to just remove it,\n> because my guess is that the compiler has gotten smarter between 2009\n> and now.\n\nAlmost certainly. I don't have a machine to test it on, either, but I\nwould be shocked if `make BLK_SHA=YesPlease DEVELOPER=1` worked on\nmaster today on an arm machine.\n\n> I honestly intend to just remove this code in a future version because\n> everyone not using SHA1DC has a security problem and we shouldn't offer\n> insecure options.\n>\n> However, I think for now, I'm just going to reroll this with the new\n> title and then I can remove it in a future version unless somebody with\n> an ARM system can relatively quickly tell me whether it's necessary.\n\nI wonder if a good stop-gap for arm systems might be to do something\nlike:\n\n--- 8< ---\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 1bb6e7c069..7402d02875 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -57,7 +57,7 @@\n #if defined(__i386__) || defined(__x86_64__)\n   #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))\n #elif defined(__GNUC__) && defined(__arm__)\n-  #define setW(x, val) do { W(x) = (val); __asm__(\"\":::\"memory\"); } while (0)\n+  #define setW(x, val) do { W(x) = (val); __extension__ __asm__(\"\":::\"memory\"); } while (0)\n #else\n   #define setW(x, val) (W(x) = (val))\n #endif\n\n--- >8 ---\n\nin the meantime in a separate patch. There it seems like the memory\nbarrier is useful for machines with fewer than 25-ish registers. Though\nobviously moot if your ultimate goal is to get rid of the block sha1\ncode.\n\nBut in the meantime, a stop-gap patch may be useful. If you use that\ndiff, feel free to forge my Signed-off-by.\n\nThanks,\nTaylor\n"},{"id":"450959","messageId":"220310.86cziulls6.gmgdl@evledraar.gmail.com","threadId":"57517","inReplyTo":"Yikq7POhuxeN1UPQ@nand.local","subject":"Re: [PATCH v2] block-sha1: remove use of assembly","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-09T23:52:31Z","receivedAt":"2022-03-10T00:00:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 09 2022, Taylor Blau wrote:\n\n> On Wed, Mar 09, 2022 at 10:10:33PM +0000, brian m. carlson wrote:\n>> On 2022-03-08 at 13:38:06, Ævar Arnfjörð Bjarmason wrote:\n>> >\n>> > On Tue, Mar 08 2022, brian m. carlson wrote:\n>> >\n>> > I think the $subject of the patch needs updating. It's not removing all\n>> > the assemply from the file, after this patch we still have the\n>> > ARM-specific assembly.\n>> >\n>> > I don't have a box to test that on, but I wonder if that also triggers\n>> > the pedantic mode?\n>> >\n>> > Perhaps:\n>> >\n>> >     block-sha1: remove superfluous i386 and x86-64 assembly\n>>\n>> I suspect it has the same problem.  My inclination is to just remove it,\n>> because my guess is that the compiler has gotten smarter between 2009\n>> and now.\n>\n> Almost certainly. I don't have a machine to test it on, either, but I\n> would be shocked if `make BLK_SHA=YesPlease DEVELOPER=1` worked on\n> master today on an arm machine.\n\nWhy is that? The -pedantic error is specifically about\n\"gnu-statement-expression\", i.e. the bracket syntax, not the inline\nassembly per-se.\n\nThe ARM assembly isn't using that, and we have other code __asm__ code\ncompiled with -pedantic. E.g. I get the __asm__ in \"compat/bswap.h\" by\ndefault, and it passes -pedantic (the code starting around line 38).\n\n>> I honestly intend to just remove this code in a future version because\n>> everyone not using SHA1DC has a security problem and we shouldn't offer\n>> insecure options.\n>>\n>> However, I think for now, I'm just going to reroll this with the new\n>> title and then I can remove it in a future version unless somebody with\n>> an ARM system can relatively quickly tell me whether it's necessary.\n>\n> I wonder if a good stop-gap for arm systems might be to do something\n> like:\n>\n> --- 8< ---\n>\n> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\n> index 1bb6e7c069..7402d02875 100644\n> --- a/block-sha1/sha1.c\n> +++ b/block-sha1/sha1.c\n> @@ -57,7 +57,7 @@\n>  #if defined(__i386__) || defined(__x86_64__)\n>    #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))\n>  #elif defined(__GNUC__) && defined(__arm__)\n> -  #define setW(x, val) do { W(x) = (val); __asm__(\"\":::\"memory\"); } while (0)\n> +  #define setW(x, val) do { W(x) = (val); __extension__ __asm__(\"\":::\"memory\"); } while (0)\n>  #else\n>    #define setW(x, val) (W(x) = (val))\n>  #endif\n>\n> --- >8 ---\n\nIsn't that __extension__ only needed *if* it warns under -pedantic,\nwhich AFAICT doesn't apply to all uses of __asm__ (but your compiler\nversion etc. may be different...).\n"},{"id":"450978","messageId":"20220310015937.tx5hgpr75at7kvrm@carlos-mbp.lan","threadId":"57517","inReplyTo":"220310.86cziulls6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] block-sha1: remove use of assembly","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-03-10T01:59:37Z","receivedAt":"2022-03-10T01:59:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Mar 10, 2022 at 12:52:31AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Mar 09 2022, Taylor Blau wrote:\n> \n> > On Wed, Mar 09, 2022 at 10:10:33PM +0000, brian m. carlson wrote:\n> >> On 2022-03-08 at 13:38:06, Ævar Arnfjörð Bjarmason wrote:\n> >> >\n> >> > On Tue, Mar 08 2022, brian m. carlson wrote:\n> >> >\n> >> > I think the $subject of the patch needs updating. It's not removing all\n> >> > the assemply from the file, after this patch we still have the\n> >> > ARM-specific assembly.\n> >> >\n> >> > I don't have a box to test that on, but I wonder if that also triggers\n> >> > the pedantic mode?\n> >> >\n> >> > Perhaps:\n> >> >\n> >> >     block-sha1: remove superfluous i386 and x86-64 assembly\n> >>\n> >> I suspect it has the same problem.  My inclination is to just remove it,\n> >> because my guess is that the compiler has gotten smarter between 2009\n> >> and now.\n> >\n> > Almost certainly. I don't have a machine to test it on, either, but I\n> > would be shocked if `make BLK_SHA=YesPlease DEVELOPER=1` worked on\n> > master today on an arm machine.\n> \n> Why is that? The -pedantic error is specifically about\n> \"gnu-statement-expression\", i.e. the bracket syntax, not the inline\n> assembly per-se.\n\nnot sure how gcc version (as mentioned elsewhere) might affect this, but\nhad built it successfully in aarch64 with gcc 4.8.4, and arm32v6 with\ngcc 10.3.1.\n\nCarlo\n"},{"id":"450981","messageId":"Yilj0qMqk5eC4TKo@nand.local","threadId":"57517","inReplyTo":"220310.86cziulls6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] block-sha1: remove use of assembly","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-10T02:34:58Z","receivedAt":"2022-03-10T02:35:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 10, 2022 at 12:52:31AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> I suspect it has the same problem.  My inclination is to just remove it,\n> >> because my guess is that the compiler has gotten smarter between 2009\n> >> and now.\n> >\n> > Almost certainly. I don't have a machine to test it on, either, but I\n> > would be shocked if `make BLK_SHA=YesPlease DEVELOPER=1` worked on\n> > master today on an arm machine.\n>\n> Why is that? The -pedantic error is specifically about\n> \"gnu-statement-expression\", i.e. the bracket syntax, not the inline\n> assembly per-se.\n>\n> The ARM assembly isn't using that, and we have other code __asm__ code\n> compiled with -pedantic. E.g. I get the __asm__ in \"compat/bswap.h\" by\n> default, and it passes -pedantic (the code starting around line 38).\n\nYou're right, I had this completely mixed up in my mind. In GitHub's\nfork there is a spot I have been working near for the past couple of\ndays where there is inline assembly right below a statement expression.\n\nThe statement expression is what causes the -pedantic builds to fail,\nnot the inline __asm__. Indeed, if you just stick a memory barrier\nanywhere in Git's codebase, we'll still compile under the DEVELOPER=1\nbuilds.\n\n> Isn't that __extension__ only needed *if* it warns under -pedantic,\n> which AFAICT doesn't apply to all uses of __asm__ (but your compiler\n> version etc. may be different...).\n\nYes, disregard that last suggestion :-).\n\nThanks,\nTaylor\n"},{"id":"451049","messageId":"20220310174750.4166514-1-sandals@crustytoothpaste.net","threadId":"57517","inReplyTo":"20220307232552.2799122-1-sandals@crustytoothpaste.net","subject":"[PATCH v3] block-sha1: remove use of obsolete x86 assembly","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-10T17:47:50Z","receivedAt":"2022-03-10T17:48:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In the block SHA-1 code, we have special assembly code for i386 and\namd64 to perform rotations with assembly.  This is supposed to help pick\nthe correct rotation operation depending on which rotation is smaller,\nwhich can help some systems perform slightly better, since any circular\nrotation can be specified as either a rotate left or a rotate right.\nHowever, this isn't needed, so we should remove it.\n\nFirst, SHA-1, like SHA-2, uses fixed constant rotates.  Thus, all\nrotation amounts are known at compile time and are in fact baked into\nthe code.  Fortunately, peephole optimizers recognize rotations\nspecified in the normal way and automatically emit the correct code,\nincluding a preference for choosing a rotate left versus a rotate right.\nThis has been the case for well over a decade, and is a standard example\nof the utility of a peephole optimizer.\n\nMoreover, all modern CPUs, with the exception of extremely limited\nembedded CPUs such as some Cortex-M processors, provide a barrel\nshifter, which lets the CPU perform rotates of any bit amount in\nconstant time.  This is valuable for many cryptographic algorithms to\nimprove performance, and is required to prevent timing attacks in\nalgorithms which use data-dependent rotations (which don't include the\nhash algorithms we use).  As a result, even though the compiler does the\ncorrect optimization, it isn't even needed here and either a left or a\nright rotate is equally acceptable.\n\nIn fact, the SHA-256 code already takes this into account and just\nwrites the simple code using an inline function to let the compiler\noptimize it for us.\n\nThe downside of using this code, however, is that it uses a GCC\nextension, which makes the compiler complain when using -pedantic unless\nit's prefixed with __extension__.  We could fix that, but since it's\nnot needed, let's just remove it.  We haven't noticed this because\nalmost everyone uses the SHA1DC code instead, but it still shows up for\nsome people.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nChanges from v2:\n* Improve subject\n\n block-sha1/sha1.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 1bb6e7c069..5974cd7dd3 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -11,27 +11,10 @@\n \n #include \"sha1.h\"\n \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- * It _can_ generate slightly smaller code (a constant of 1 is special), but\n- * perhaps more importantly it's possibly faster on any uarch that does a\n- * rotate with a loop.\n- */\n-\n-#define SHA_ASM(op, x, n) ({ unsigned int __res; __asm__(op \" %1,%0\":\"=r\" (__res):\"i\" (n), \"0\" (x)); __res; })\n-#define SHA_ROL(x,n)\tSHA_ASM(\"rol\", x, n)\n-#define SHA_ROR(x,n)\tSHA_ASM(\"ror\", x, n)\n-\n-#else\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 \n-#endif\n-\n /*\n  * If you have 32 registers or more, the compiler can (and should)\n  * try to change the array[] accesses into registers. However, on\n"},{"id":"451070","messageId":"xmqqy21hd3ao.fsf@gitster.g","threadId":"57517","inReplyTo":"20220310174750.4166514-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3] block-sha1: remove use of obsolete x86 assembly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-10T19:18:39Z","receivedAt":"2022-03-10T19:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> Changes from v2:\n> * Improve subject\n\nWill queue.  Let's merge it down to 'next'.\n"}]}