[PATCH lt/block-sha1 0/2 v3] block-sha1: avoid pointer conversion that violates alignment constraints

Hi Junio,

This is a series of two patches: the first avoids alignment faults
that were making git either slow on Alpha machines or crashy,
depending on the machine's configuration, and the second patch is a
cosmetic nit noticed while reviewing the first.

Patches are based against

  30ae47b4 remove ARM and Mozilla SHA1 implementations, 2009-08-17

(aka the tip of the lt/block-sha1 branch) for no particular reason.
They should work fine against a more modern codebase if you prefer
that.

No changes since v2[1] except to commit messages.

I think this should be ready for application.  Thanks to Michael and
Linus for shaping the original patch into something sane.

Thoughts?
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/201434/focus=201456

Jonathan Nieder (2):
  block-sha1: avoid pointer conversion that violates alignment
    constraints
  block-sha1: put expanded macro parameters in parentheses

 block-sha1/sha1.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

With 660231aa (block-sha1: support for architectures with memory
alignment restrictions, 2009-08-12), blk_SHA1_Update was modified to
access 32-bit chunks of memory one byte at a time on arches that
prefer that:

	#define get_be32(p)    ( \
		(*((unsigned char *)(p) + 0) << 24) | \
		(*((unsigned char *)(p) + 1) << 16) | \
		(*((unsigned char *)(p) + 2) <<  8) | \
		(*((unsigned char *)(p) + 3) <<  0) )

The code previously accessed these values by just using htonl(*p).

Unfortunately, Michael noticed on an Alpha machine that git was using
plain 32-bit reads anyway.  As soon as we convert a pointer to int *,
the compiler can assume that the object pointed to is correctly
aligned as an int (C99 section 6.3.2.3 "pointer conversions"
paragraph 7), and gcc takes full advantage by using a single 32-bit
load, resulting in a whole bunch of unaligned access traps.

So we need to obey the alignment constraints even when only dealing
with pointers instead of actual values.  Do so by changing the type
of 'data' to void *.  This patch renames 'data' to 'block' at the same
time to make sure all references are updated to reflect the new type.

Reported-tested-and-explained-by: Michael Cree [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
---
Changes since v2:

 - fixed explanation of how the alignment assumption shows up in the
   assembler[1]
 - add Linus's ack

[1] http://thread.gmane.org/gmane.comp.version-control.git/201434/focus=201484

 block-sha1/sha1.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index d8934757..10fd94d1 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c

@@ -100,7 +100,7 @@
  * Where do we get the source from? The first 16 iterations get it from
  * the input data, the next mix it from the 512-bit array.
  */
-#define SHA_SRC(t) get_be32(data + t)
+#define SHA_SRC(t) get_be32((unsigned char *) block + t*4)
 #define SHA_MIX(t) SHA_ROL(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
 
 #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \

@@ -114,7 +114,7 @@
 #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 )
 #define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) ,  0xca62c1d6, A, B, C, D, E )
 
-static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned int *data)
+static void blk_SHA1_Block(blk_SHA_CTX *ctx, const void *block)
 {
 	unsigned int A,B,C,D,E;
 	unsigned int array[16];

@@ -125,7 +125,7 @@ static void blk_SHA1_Block(blk_SHA_CTX *ctx, const unsigned int *data)
 	D = ctx->H[3];
 	E = ctx->H[4];
 
-	/* Round 1 - iterations 0-16 take their input from 'data' */
+	/* Round 1 - iterations 0-16 take their input from 'block' */
 	T_0_15( 0, A, B, C, D, E);
 	T_0_15( 1, E, A, B, C, D);
 	T_0_15( 2, D, E, A, B, C);

-- 
1.7.10.4

't' is currently always a numeric constant, but it can't hurt to
prepare for the day that it becomes useful for a caller to pass in a
more complex expression.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <redacted>
---
Clarified subject line.  No other change.

Thanks for reading.

 block-sha1/sha1.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index 10fd94d1..6f885c43 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c

@@ -100,8 +100,8 @@
  * Where do we get the source from? The first 16 iterations get it from
  * the input data, the next mix it from the 512-bit array.
  */
-#define SHA_SRC(t) get_be32((unsigned char *) block + t*4)
-#define SHA_MIX(t) SHA_ROL(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
+#define SHA_SRC(t) get_be32((unsigned char *) block + (t)*4)
+#define SHA_MIX(t) SHA_ROL(W((t)+13) ^ W((t)+8) ^ W((t)+2) ^ W(t), 1);
 
 #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
 	unsigned int TEMP = input(t); setW(t, TEMP); \

-- 
1.7.10.4