git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Michael Cree <mcree@orcon.net.nz>
Cc: git@vger.kernel.org, Nicolas Pitre <nico@fluxnic.net>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: [PATCH maint-1.6.5] block-sha1: avoid unaligned accesses on some big-endian systems
Date: Sat, 14 Jul 2012 02:59:06 -0500	[thread overview]
Message-ID: <20120714075906.GD3693@burratino> (raw)
In-Reply-To: <50010B84.5030606@orcon.net.nz>

With 660231aa (block-sha1: support for architectures with memory
alignment restrictions, 2009-08-12), blk_SHA1_Update was modified to
work reasonably on RISC-y architectures by accessing 32-bit values one
byte at a time on machines that prefer that:

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

Unfortunately, on big-endian architectures, if p is a pointer to
unsigned int then current gcc assumes it is properly aligned and
converts this construct to a 32-bit load.  The result is the same
alignment fault we want to avoid.  Fix it by passing around 'data' as
an unsigned char * instead of unsigned int * to more clearly document
the intended alignment for compilers and humans.

The patch renames 'data' to 'block' to make sure all references are
updated to reflect its new type.

Noticed on an alpha.  Typical ARM systems and other little-endian
arches don't hit this because with the endianness conversion gcc
(rightly) prefers reading one byte at a time even if the pointer is
aligned.

Reported-and-tested-by: Michael Cree <mcree@orcon.net.nz>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Michael Cree wrote[1]:
> On 14/07/12 14:18, Jonathan Nieder wrote:

>> Does this patch help?
>
> Yes, it does!  Unaligned accesses are no longer reported and git is
> running noticeably faster.

Thanks!  Nico, Linus, what do you think?

As Michael mentioned, in the long term it might make sense to tweak
git to always pass aligned pointers and lengths in the 'data' and
'len' arguments to SHA1_Update, but that's an experiment for another
day.

[1] http://bugs.debian.org/681532

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

diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index d8934757..4d72e4bc 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(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 unsigned char *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);
@@ -251,7 +251,7 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
 		data = ((const char *)data + left);
 		if (lenW)
 			return;
-		blk_SHA1_Block(ctx, ctx->W);
+		blk_SHA1_Block(ctx, (const unsigned char *) ctx->W);
 	}
 	while (len >= 64) {
 		blk_SHA1_Block(ctx, data);
-- 
1.7.10.4

       reply	other threads:[~2012-07-14  7:59 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20120713233957.6928.87541.reportbug@electro.phys.waikato.ac.nz>
     [not found] ` <20120714002950.GA3159@burratino>
     [not found]   ` <5000CBCA.8020607@orcon.net.nz>
     [not found]     ` <20120714021856.GA3062@burratino>
     [not found]       ` <50010B84.5030606@orcon.net.nz>
2012-07-14  7:59         ` Jonathan Nieder [this message]
2012-07-14 19:11           ` [PATCH maint-1.6.5] block-sha1: avoid unaligned accesses on some big-endian systems Linus Torvalds
2012-07-14 19:50             ` Jonathan Nieder
2012-07-14 19:55               ` Linus Torvalds
2012-07-14 20:50             ` [PATCH maint-1.6.5 v2] block-sha1: avoid pointer conversion that violates alignment constraints Jonathan Nieder
2012-07-14 20:56               ` Linus Torvalds
2012-07-14 21:57                 ` [PATCH] block-sha1: put macro arguments in parentheses Jonathan Nieder
2012-07-15 20:58               ` [PATCH maint-1.6.5 v2] block-sha1: avoid pointer conversion that violates alignment constraints Michael Cree
2012-07-15 21:27                 ` Jonathan Nieder
2012-07-16  9:53                   ` Michael Cree

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120714075906.GD3693@burratino \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=mcree@orcon.net.nz \
    --cc=nico@fluxnic.net \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).