{"thread":{"id":"31069","subject":"[PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","startedAt":"2012-07-22T23:35:47Z","lastAt":"2012-07-23T06:29:14Z","messageCount":9,"participants":["Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"195465","messageId":"20120722233547.GA1978@burratino","threadId":"31069","inReplyTo":null,"subject":"[PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-22T23:35:47Z","receivedAt":"2012-07-22T23:35:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Junio,\n\nThis is a series of two patches: the first avoids alignment faults\nthat were making git either slow on Alpha machines or crashy,\ndepending on the machine's configuration, and the second patch is a\ncosmetic nit noticed while reviewing the first.\n\nPatches are based against\n\n 30ae47b4 remove ARM and Mozilla SHA1 implementations, 2009-08-17\n\n(aka the tip of the lt/block-sha1 branch) for no particular reason.\nThey should work fine against a more modern codebase if you prefer\nthat.\n\nNo changes since v2[1] except to commit messages.\n\nI think this should be ready for application. Thanks to Michael and\nLinus for shaping the original patch into something sane.\n\nThoughts?\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/201434/focus=201456\n\nJonathan Nieder (2):\n block-sha1: avoid pointer conversion that violates alignment\n constraints\n block-sha1: put expanded macro parameters in parentheses\n\n block-sha1/sha1.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n"},{"id":"195466","messageId":"20120722233954.GA2012@burratino","threadId":"31069","inReplyTo":"20120722233547.GA1978@burratino","subject":"[PATCH 1/2] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-22T23:39:54Z","receivedAt":"2012-07-22T23:39:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"With 660231aa (block-sha1: support for architectures with memory\nalignment restrictions, 2009-08-12), blk_SHA1_Update was modified to\naccess 32-bit chunks of memory one byte at a time on arches that\nprefer that:\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\nThe code previously accessed these values by just using htonl(*p).\n\nUnfortunately, Michael noticed on an Alpha machine that git was using\nplain 32-bit reads anyway. As soon as we convert a pointer to int *,\nthe compiler can assume that the object pointed to is correctly\naligned as an int (C99 section 6.3.2.3 \"pointer conversions\"\nparagraph 7), and gcc takes full advantage by using a single 32-bit\nload, resulting in a whole bunch of unaligned access traps.\n\nSo we need to obey the alignment constraints even when only dealing\nwith pointers instead of actual values. Do so by changing the type\nof 'data' to void *. This patch renames 'data' to 'block' at the same\ntime to make sure all references are updated to reflect the new type.\n\nReported-tested-and-explained-by: Michael Cree \nSigned-off-by: Jonathan Nieder \nAcked-by: Linus Torvalds \n---\nChanges since v2:\n\n - fixed explanation of how the alignment assumption shows up in the\n assembler[1]\n - add Linus's ack\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/201434/focus=201484\n\n block-sha1/sha1.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex d8934757..10fd94d1 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -100,7 +100,7 @@\n * Where do we get the source from? The first 16 iterations get it from\n * the input data, the next mix it from the 512-bit array.\n */\n-#define SHA_SRC(t) get_be32(data + t)\n+#define SHA_SRC(t) get_be32((unsigned char *) block + t*4)\n #define SHA_MIX(t) SHA_ROL(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)\n \n #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \\\n@@ -114,7 +114,7 @@\n #define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B&C)+(D&(B^C))) , 0x8f1bbcdc, A, B, C, D, E )\n #define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) , 0xca62c1d6, A, B, C, D, E )\n \n-static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned int *data)\n+static void blk_SHA1_Block(blk_SHA_CTX *ctx, const void *block)\n {\n \tunsigned int A,B,C,D,E;\n \tunsigned int array[16];\n@@ -125,7 +125,7 @@ static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned int *data)\n \tD = ctx->H[3];\n \tE = ctx->H[4];\n \n-\t/* Round 1 - iterations 0-16 take their input from 'data' */\n+\t/* Round 1 - iterations 0-16 take their input from 'block' */\n \tT_0_15( 0, A, B, C, D, E);\n \tT_0_15( 1, E, A, B, C, D);\n \tT_0_15( 2, D, E, A, B, C);\n-- \n1.7.10.4\n"},{"id":"195467","messageId":"20120722234054.GB2012@burratino","threadId":"31069","inReplyTo":"20120722233547.GA1978@burratino","subject":"[PATCH 2/2] block-sha1: put expanded macro parameters in parentheses","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-22T23:40:54Z","receivedAt":"2012-07-22T23:40:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"'t' is currently always a numeric constant, but it can't hurt to\nprepare for the day that it becomes useful for a caller to pass in a\nmore complex expression.\n\nSuggested-by: Linus Torvalds \nSigned-off-by: Jonathan Nieder \n---\nClarified subject line. No other change.\n\nThanks for reading.\n\n block-sha1/sha1.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/block-sha1/sha1.c b/block-sha1/sha1.c\nindex 10fd94d1..6f885c43 100644\n--- a/block-sha1/sha1.c\n+++ b/block-sha1/sha1.c\n@@ -100,8 +100,8 @@\n * Where do we get the source from? The first 16 iterations get it from\n * the input data, the next mix it from the 512-bit array.\n */\n-#define SHA_SRC(t) get_be32((unsigned char *) block + t*4)\n-#define SHA_MIX(t) SHA_ROL(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)\n+#define SHA_SRC(t) get_be32((unsigned char *) block + (t)*4)\n+#define SHA_MIX(t) SHA_ROL(W((t)+13) ^ W((t)+8) ^ W((t)+2) ^ W(t), 1);\n \n #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \\\n \tunsigned int TEMP = input(t); setW(t, TEMP); \\\n-- \n1.7.10.4\n"},{"id":"195486","messageId":"7v8vebp0cl.fsf@alter.siamese.dyndns.org","threadId":"31069","inReplyTo":"20120722233547.GA1978@burratino","subject":"Re: [PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T04:28:42Z","receivedAt":"2012-07-23T04:28:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> This is a series of two patches: the first avoids alignment faults\n> that were making git either slow on Alpha machines or crashy,\n> depending on the machine's configuration, and the second patch is a\n> cosmetic nit noticed while reviewing the first.\n> ...\n> Thoughts?\n> Jonathan\n\nThanks.\n\nDid somebody actually compiled Git for Alpha, and even more\nsurprisingly on a big-endian variant of one?\n"},{"id":"195488","messageId":"20120723045148.GA13623@burratino","threadId":"31069","inReplyTo":"7v8vebp0cl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-23T04:51:48Z","receivedAt":"2012-07-23T04:51:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Thanks.\n>\n> Did somebody actually compiled Git for Alpha, and even more\n> surprisingly on a big-endian variant of one?\n\nLogs from building for Alpha and running the test suite are here:\n\n http://buildd.debian-ports.org/status/logs.php?pkg=git&arch=alpha\n\nThe big-endian part was just my idiocy, sorry.\n"},{"id":"195490","messageId":"7vobn7njtz.fsf@alter.siamese.dyndns.org","threadId":"31069","inReplyTo":"20120723045148.GA13623@burratino","subject":"Re: [PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T05:10:48Z","receivedAt":"2012-07-23T05:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> Thanks.\n>>\n>> Did somebody actually compiled Git for Alpha, and even more\n>> surprisingly on a big-endian variant of one?\n>\n> Logs from building for Alpha and running the test suite are here:\n>\n> http://buildd.debian-ports.org/status/logs.php?pkg=git&arch=alpha\n>\n> The big-endian part was just my idiocy, sorry.\n\nHrm, do we want an update log message for 1/2 then?\n"},{"id":"195494","messageId":"20120723052605.GA13728@burratino","threadId":"31069","inReplyTo":"7vobn7njtz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-23T05:28:02Z","receivedAt":"2012-07-23T05:28:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> The big-endian part was just my idiocy, sorry.\n>\n> Hrm, do we want an update log message for 1/2 then?\n\nHm, I thought all the crazy had been eliminated already.\n\n*looks again*\n\nI guess \"using a single 32-bit load\" makes it sound like it's using a\nbig-endian load instead of a load followed by twiddling in registers.\n\nSimplest fix would be to drop the phrase \"by using a single 32-bit\nload\", leaving \"... and gcc takes full advantage, resulting in a whole\nbunch of unaligned access traps.\" Would that work for you, or should\nI resend?\n\nThanks,\nJonathan\n"},{"id":"195496","messageId":"7vboj7nidc.fsf@alter.siamese.dyndns.org","threadId":"31069","inReplyTo":"20120723052605.GA13728@burratino","subject":"Re: [PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-23T05:42:23Z","receivedAt":"2012-07-23T05:42:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>> The big-endian part was just my idiocy, sorry.\n>>\n>> Hrm, do we want an update log message for 1/2 then?\n>\n> Hm, I thought all the crazy had been eliminated already.\n>\n> *looks again*\n>\n> I guess \"using a single 32-bit load\" makes it sound like it's using a\n> big-endian load instead of a load followed by twiddling in registers.\n>\n> Simplest fix would be to drop the phrase \"by using a single 32-bit\n> load\", leaving \"... and gcc takes full advantage, resulting in a whole\n> bunch of unaligned access traps.\"\n\nI was just being stupid, misread the original code and did not\nrealize that the generic code quoted in your log message:\n\n\t#define get_be32(p) ( \\\n \t... byte-at-a-time implementation )\n\nis not limited to big endian boxes [*1*]. So I think the message is\nfine as-is.\n\n\n[Footnote]\n\n*1* In other words, the ntohl(*(uint *)(p)) is used only on selected\nlittle endian boxes, but that does not mean the generic code was\nbig-endian only. Little endian boxes that are not listed in #if\nblock do use the generic byte-at-a-time macro.\n"},{"id":"195501","messageId":"20120723062914.GA14045@burratino","threadId":"31069","inReplyTo":"7vboj7nidc.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC 3/1] Makefile: BLK_SHA1 does not require fast htonl() and unaligned loads","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-23T06:29:14Z","receivedAt":"2012-07-23T06:29:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"block-sha1/ is fast on most known platforms. Clarify the Makefile to\nbe less misleading about that.\n\nEarly versions of block-sha1/ explicitly relied on fast htonl() and\nfast 32-bit loads with arbitrary alignment. Now it uses those on some\narches but the default behavior is byte-at-a-time access for the sake\nof arches like ARM, Alpha, and their kin and it is still pretty fast\non these arches (fast enough to supersede the mozilla SHA1\nimplementation and the hand-written ARM assembler implementation that\nwere bundled before).\n\nSigned-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n\n> *1* In other words, the ntohl(*(uint *)(p)) is used only on selected\n> little endian boxes, but that does not mean the generic code was\n> big-endian only. Little endian boxes that are not listed in #if\n> block do use the generic byte-at-a-time macro.\n\nOh, that reminds me. Here's a third patch.\n\n Makefile | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 134606b9..eadcc70a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -84,9 +84,8 @@ all::\n # specify your own (or DarwinPort's) include directories and\n # library directories by defining CFLAGS and LDFLAGS appropriately.\n #\n-# Define BLK_SHA1 environment variable if you want the C version\n-# of the SHA1 that assumes you can do unaligned 32-bit loads and\n-# have a fast htonl() function.\n+# Define BLK_SHA1 environment variable to make use of the bundled\n+# optimized C SHA1 routine.\n #\n # Define PPC_SHA1 environment variable when running make to make use of\n # a bundled SHA1 routine optimized for PowerPC.\n-- \n1.7.10.4\n"}]}