[PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[David Aguilar]

Mac OS X Mountain Lion prints warnings when building git:

	warning: 'SHA1_Init' is deprecated
	(declared at /usr/include/openssl/sha.h:121)

Silence the warnings by using the Common Digest SHA-1
functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

Add a COMMON_DIGEST_SHA1 knob to the Makefile to allow
choosing this implementation and define it by default on Darwin.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
The previous version of this patch (somehow?) eliminated the SHA1
warnings, but it was not redefining SHA1_HEADER to CommonDigest.h.
This version goes all the way.

Some warnings still exist around the HMAC functions (and others)
used by e.g. imap.c, which should be dealt with separately.

 Makefile | 7 +++++++
 cache.h  | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/Makefile b/Makefile
index 0f931a2..d8a45b4 100644
--- a/Makefile
+++ b/Makefile
@@ -1054,6 +1054,7 @@ ifeq ($(uname_S),Darwin)
 			BASIC_LDFLAGS += -L/opt/local/lib
 		endif
 	endif
+	COMMON_DIGEST_SHA1 = YesPlease
 	PTHREAD_LIBS =
 endif
 
@@ -1388,10 +1389,16 @@ ifdef PPC_SHA1
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
 	LIB_H += ppc/sha1.h
 else
+ifdef COMMON_DIGEST_SHA1
+	BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_SHA1=1
+	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
+	EXTLIBS += $(LIB_4_CRYPTO)
+else
 	SHA1_HEADER = <openssl/sha.h>
 	EXTLIBS += $(LIB_4_CRYPTO)
 endif
 endif
+endif
 ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif
diff --git a/cache.h b/cache.h
index 94ca1ac..e2b24c6 100644
--- a/cache.h
+++ b/cache.h
@@ -10,11 +10,18 @@
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
+#ifdef COMMON_DIGEST_FOR_SHA1
+#define git_SHA_CTX	CC_SHA1_CTX
+#define git_SHA1_Init	CC_SHA1_Init
+#define git_SHA1_Update	CC_SHA1_Update
+#define git_SHA1_Final	CC_SHA1_Final
+#else
 #define git_SHA_CTX	SHA_CTX
 #define git_SHA1_Init	SHA1_Init
 #define git_SHA1_Update	SHA1_Update
 #define git_SHA1_Final	SHA1_Final
 #endif
+#endif
 
 #include <zlib.h>
 typedef struct git_zstream {
-- 
1.8.3.rc1.45.g88f80b8.dirty

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[Jonathan Nieder]

Hi,

David Aguilar wrote:

> Mac OS X Mountain Lion prints warnings when building git:
>
> 	warning: 'SHA1_Init' is deprecated
> 	(declared at /usr/include/openssl/sha.h:121)
>
> Silence the warnings by using the Common Digest SHA-1
> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

Thanks.

Does this perform better or worse than just setting
BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
hand adding another library dependency can slow down startup, and on
the other hand the implementation may or may not be optimized better
than the generic block-sha1/ implementation.

Curious,
Jonathan

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[David Aguilar]

On Fri, May 10, 2013 at 11:23 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> David Aguilar wrote:
>
>> Mac OS X Mountain Lion prints warnings when building git:
>>
>>       warning: 'SHA1_Init' is deprecated
>>       (declared at /usr/include/openssl/sha.h:121)
>>
>> Silence the warnings by using the Common Digest SHA-1
>> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().
>
> Thanks.
>
> Does this perform better or worse than just setting
> BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
> hand adding another library dependency can slow down startup, and on
> the other hand the implementation may or may not be optimized better
> than the generic block-sha1/ implementation.

Pretty much identical.

Here are the timings (I should probably read t/perf/README and get
better numbers):

Best of ten
$ time git rev-list --all --objects >/dev/null

# master
git rev-list --all --objects > /dev/null  5.16s user 0.11s system 99%
cpu 5.277 total

# BLK_SHA1
git rev-list --all --objects > /dev/null  5.17s user 0.11s system 99%
cpu 5.299 total

# CommonDigest
git rev-list --all --objects > /dev/null  5.18s user 0.11s system 99%
cpu 5.301 total

The library startup cost is identical to master since at the end of
the day it's still libcrypto.

FWIW the way it's done in this patch still allows defining BLK_SHA1,
which is parity with how the Makefile behaves on Linux.

Re-roll?
--
David

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[Jeff King]

On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote:

> > Does this perform better or worse than just setting
> > BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
> > hand adding another library dependency can slow down startup, and on
> > the other hand the implementation may or may not be optimized better
> > than the generic block-sha1/ implementation.
> 
> Pretty much identical.
> 
> Here are the timings (I should probably read t/perf/README and get
> better numbers):
> 
> Best of ten
> $ time git rev-list --all --objects >/dev/null
> [...]

I'm not sure that's a great test of sha1 performance. It will hash the
commit and tree objects it loads during the traversal, but that time is
almost certainly dwarfed by zlib inflation and by lookup_object.

Adding "--verify-objects" would sha1 the blobs, too, which might be more
reasonable (or running "git fsck"). Something like "git add" on a large
blob would also be a good test.

-Peff

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[David Aguilar]

On Sat, May 11, 2013 at 1:22 AM, Jeff King <peff@peff.net> wrote:
> On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote:
>
>> > Does this perform better or worse than just setting
>> > BLK_SHA1=YesPlease?  I'd naively think it could go either way: on one
>> > hand adding another library dependency can slow down startup, and on
>> > the other hand the implementation may or may not be optimized better
>> > than the generic block-sha1/ implementation.
>>
>> Pretty much identical.
>>
>> Here are the timings (I should probably read t/perf/README and get
>> better numbers):
>>
>> Best of ten
>> $ time git rev-list --all --objects >/dev/null
>> [...]
>
> I'm not sure that's a great test of sha1 performance. It will hash the
> commit and tree objects it loads during the traversal, but that time is
> almost certainly dwarfed by zlib inflation and by lookup_object.
>
> Adding "--verify-objects" would sha1 the blobs, too, which might be more
> reasonable (or running "git fsck"). Something like "git add" on a large
> blob would also be a good test.

Thanks.  Here are the numbers with --verify-objects:

$ time git rev-list --all --objects --verify-objects >/dev/null

# CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total
# master       33.00s user 4.68s system 99% cpu 37.852 total
# BLK_SHA1     54.17s user 4.67s system 99% cpu 58.928 total

Doing BLK_SHA1 seems like less of a good idea now, so I think my
latest re-roll might be the way to go...
--
David

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[Jeff King]

On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote:

> > Adding "--verify-objects" would sha1 the blobs, too, which might be more
> > reasonable (or running "git fsck"). Something like "git add" on a large
> > blob would also be a good test.
> 
> Thanks.  Here are the numbers with --verify-objects:
> 
> $ time git rev-list --all --objects --verify-objects >/dev/null
> 
> # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total
> # master       33.00s user 4.68s system 99% cpu 37.852 total
> # BLK_SHA1     54.17s user 4.67s system 99% cpu 58.928 total
> 
> Doing BLK_SHA1 seems like less of a good idea now, so I think my
> latest re-roll might be the way to go...

Wow, that's really terrible. What level of optimization do you compile
with? With the other implementations, you are calling into
(presumably) optimized library code, but with BLK_SHA1 you are getting
whatever you just compiled.

Here are three timings that show how big a difference that can make:

  openssl,  -O0    0m21.152s
  BLK_SHA1, -O0    0m31.920s
  BLK_SHA1, -O3    0m19.652s

So it is a win over openssl with optimizations on, but a pretty big loss
otherwise.

-Peff

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[David Aguilar]

On Sat, May 11, 2013 at 1:45 AM, Jeff King <peff@peff.net> wrote:
> On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote:
>
>> > Adding "--verify-objects" would sha1 the blobs, too, which might be more
>> > reasonable (or running "git fsck"). Something like "git add" on a large
>> > blob would also be a good test.
>>
>> Thanks.  Here are the numbers with --verify-objects:
>>
>> $ time git rev-list --all --objects --verify-objects >/dev/null
>>
>> # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total
>> # master       33.00s user 4.68s system 99% cpu 37.852 total
>> # BLK_SHA1     54.17s user 4.67s system 99% cpu 58.928 total
>>
>> Doing BLK_SHA1 seems like less of a good idea now, so I think my
>> latest re-roll might be the way to go...
>
> Wow, that's really terrible. What level of optimization do you compile
> with? With the other implementations, you are calling into
> (presumably) optimized library code, but with BLK_SHA1 you are getting
> whatever you just compiled.
>
> Here are three timings that show how big a difference that can make:
>
>   openssl,  -O0    0m21.152s
>   BLK_SHA1, -O0    0m31.920s
>   BLK_SHA1, -O3    0m19.652s
>
> So it is a win over openssl with optimizations on, but a pretty big loss
> otherwise.

Good catch.  I had a config.mak without any -O flags in CFLAGS.
Here are the timings with -O3.  We're back to parity.

$ time git rev-list --all --objects --verify-objects >/dev/null

# CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total
# master       29.81s user 4.70s system 99% cpu 34.760 total
# BLK_SHA1     29.80s user 4.62s system 99% cpu 34.505 total

If BLK_SHA1 were the default on all platforms then I wouldn't have
bothered with the SHA-1 patch.  With this patch it makes it like Linux
in that Git can choose between the built-in functions and the external
library.

That's why I moved this patch to 3/3.. it could go either way.
--
David

Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8

[Jeff King]

On Sat, May 11, 2013 at 02:17:10AM -0700, David Aguilar wrote:

> Good catch.  I had a config.mak without any -O flags in CFLAGS.
> Here are the timings with -O3.  We're back to parity.
> 
> $ time git rev-list --all --objects --verify-objects >/dev/null
> 
> # CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total
> # master       29.81s user 4.70s system 99% cpu 34.760 total
> # BLK_SHA1     29.80s user 4.62s system 99% cpu 34.505 total
> 
> If BLK_SHA1 were the default on all platforms then I wouldn't have
> bothered with the SHA-1 patch.  With this patch it makes it like Linux
> in that Git can choose between the built-in functions and the external
> library.

With those numbers above, it seems like CommonCrypto is still a
reasonable choice. I don't know anything about availability or other OS
X specific issues, but assuming there are no issues there, your patch
makes sense to me. Thanks for double-checking the timings.

-Peff