From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Mike Hommey" <mh@glandium.org>,
"brian m . carlson" <sandals@crustytoothpaste.net>,
"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
"Taylor Blau" <me@ttaylorr.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 0/2] Makefile + hash.h: remove PPC_SHA1 implementation
Date: Wed, 31 Aug 2022 11:18:42 +0200 [thread overview]
Message-ID: <cover-v3-0.2-00000000000-20220831T090744Z-avarab@gmail.com> (raw)
In-Reply-To: <patch-v2-1.1-e77fd23a824-20220321T170412Z-avarab@gmail.com>
This is a re-roll of [1] sent back in March. It removes the PPC_SHA1
implementation, which as noted in in 1/2 is likely completely unused
at this point, if anyone's using it we have other better tested (and
just as fast) SHA-1 implementations.
I then included this change in a larger series sent in late April[2],
which stalled for lack of feedback from the OSX crowd[3].
But this part should be uncontroversial, and is an obvious win in
terms of the diffstat, and in reducing our complexity surface in this
area.
Changes since v2:
* Much rewritten commit message (see range-diff)
* Rebased on upstream changes
* More PPC_SHA1 removal of comments/docs that I missed the first time
around.
* The s/C_OBJS/OBJECTS/ Makefile change is now split into a new 2/2.
* Added brian's Acked-by, per [4] (not explicitly an "Acked-by", but
I thought adding amounted to a fair paraphrasing of the feedback
there)
1. https://lore.kernel.org/git/patch-v2-1.1-e77fd23a824-20220321T170412Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-0.5-00000000000-20220422T094624Z-avarab@gmail.com/
3. https://lore.kernel.org/git/xmqqv8v17xrl.fsf@gitster.g/
4. https://lore.kernel.org/git/Yjjr9fkybVmB53M7@camp.crustytoothpaste.net/
Ævar Arnfjörð Bjarmason (2):
Makefile + hash.h: remove PPC_SHA1 implementation
Makefile: use $(OBJECTS) instead of $(C_OBJ)
INSTALL | 3 +-
Makefile | 22 ++---
block-sha1/sha1.c | 4 -
configure.ac | 3 -
hash.h | 6 +-
ppc/sha1.c | 72 ---------------
ppc/sha1.h | 25 ------
ppc/sha1ppc.S | 224 ----------------------------------------------
8 files changed, 9 insertions(+), 350 deletions(-)
delete mode 100644 ppc/sha1.c
delete mode 100644 ppc/sha1.h
delete mode 100644 ppc/sha1ppc.S
Range-diff against v2:
1: e77fd23a824 ! 1: 87a204b8937 ppc: remove custom SHA-1 implementation
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- ppc: remove custom SHA-1 implementation
+ Makefile + hash.h: remove PPC_SHA1 implementation
Remove the PPC_SHA1 implementation added in a6ef3518f9a ([PATCH] PPC
assembly implementation of SHA1, 2005-04-22). When this was added
Apple consumer hardware used the PPC architecture, and the
implementation was intended to improve SHA-1 speed there.
- Since it was added we've moved to DC_SHA1 by default, and anyone
- wanting hard-rolled non-DC SHA-1 implementation can use OpenSSL's via
- the OPENSSL_SHA1 knob.
+ Since it was added we've moved to using sha1collisiondetection by
+ default, and anyone wanting hard-rolled non-DC SHA-1 implementation
+ can use OpenSSL's via the OPENSSL_SHA1 knob.
- I'm unsure if this was ever supposed to work on 64-bit PPC. It clearly
- originally targeted 32 bit PPC, but there's some mailing list
- references to this being tried on G5 (PPC 970). I can't get it to do
- anything but segfault on the BE POWER8 machine in the GCC compile
- farm. Anyone caring about speed on PPC these days is likely to be
- using IBM's POWER, not PPC 970.
+ The PPC_SHA1 originally originally targeted 32 bit PPC, and later the
+ 64 bit PPC 970 (a.k.a. Apple PowerPC G5). See 926172c5e48 (block-sha1:
+ improve code on large-register-set machines, 2009-08-10) for a
+ reference about the performance on G5 (a comment in block-sha1/sha1.c
+ being removed here).
- There have been proposals to entirely remove non-DC_SHA1
- implementations from the tree[1]. I think per [2] that would be a bit
+ I can't get it to do anything but segfault on both the BE and LE POWER
+ machines in the GCC compile farm[1]. Anyone who's concerned about
+ performance on PPC these days is likely to be using the IBM POWER
+ processors.
+
+ There have been proposals to entirely remove non-sha1collisiondetection
+ implementations from the tree[2]. I think per [3] that would be a bit
overzealous. I.e. there are various set-ups git's speed is going to be
more important than the relatively implausible SHA-1 collision attack,
or where such attacks are entirely mitigated by other means (e.g. by
incoming objects being checked with DC_SHA1).
- The main reason for doing so at this point is to simplify follow-up
- Makefile change. Since PPC_SHA1 included the only in-tree *.S assembly
- file we needed to keep around special support for building objects
- from it. By getting rid of it we know we'll always build *.o from *.c
- files, which makes the build process simpler.
+ But that really doesn't apply to PPC_SHA1 in particular, which seems
+ to have outlived its usefulness.
+
+ As this gets rid of the only in-tree *.S assembly file we can remove
+ the small bits of logic from the Makefile needed to build objects
+ from *.S (as opposed to *.c)
- As an aside the code being removed here was also throwing warnings
- with the "-pedantic" flag, but let's remove it instead of fixing it,
- as 544d93bc3b4 (block-sha1: remove use of obsolete x86 assembly,
- 2022-03-10) did for block-sha1/*.
+ The code being removed here was also throwing warnings with the
+ "-pedantic" flag, it could have been fixed as 544d93bc3b4 (block-sha1:
+ remove use of obsolete x86 assembly, 2022-03-10) did for block-sha1/*,
+ but as noted above let's remove it instead.
- 1. https://lore.kernel.org/git/20200223223758.120941-1-mh@glandium.org/
- 2. https://lore.kernel.org/git/20200224044732.GK1018190@coredump.intra.peff.net/
+ 1. https://cfarm.tetaneutral.net/machines/list/
+ Tested on gcc{110,112,135,203}, a mixture of POWER [789] ppc64 and
+ ppc64le. All segfault in anything needing object
+ hashing (e.g. t/t1007-hash-object.sh) when compiled with
+ PPC_SHA1=Y.
+ 2. https://lore.kernel.org/git/20200223223758.120941-1-mh@glandium.org/
+ 3. https://lore.kernel.org/git/20200224044732.GK1018190@coredump.intra.peff.net/
+ Acked-by: brian m. carlson" <sandals@crustytoothpaste.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## INSTALL ##
@@ Makefile: include shared.mak
# Define DC_SHA1 to unconditionally enable the collision-detecting sha1
# algorithm. This is slower, but may detect attempted collision attacks.
# Takes priority over other *_SHA1 knobs.
-@@ Makefile: ifdef OPENSSL_SHA1
- EXTLIBS += $(LIB_4_CRYPTO)
- BASIC_CFLAGS += -DSHA1_OPENSSL
- else
+@@ Makefile: ifdef APPLE_COMMON_CRYPTO
+ SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L
+ endif
+
+ifdef PPC_SHA1
-+$(error PPC_SHA1 has been removed! Use DC_SHA1 instead, which is the default)
++$(error the PPC_SHA1 flag has been removed along with the PowerPC-specific SHA-1 implementation.)
+endif
- ifdef BLK_SHA1
++
+ ifdef OPENSSL_SHA1
+ EXTLIBS += $(LIB_4_CRYPTO)
+ BASIC_CFLAGS += -DSHA1_OPENSSL
+@@ Makefile: ifdef BLK_SHA1
LIB_OBJS += block-sha1/sha1.o
BASIC_CFLAGS += -DSHA1_BLK
else
@@ Makefile: missing_compdb_dir =
-ASM_SRC := $(wildcard $(OBJECTS:o=S))
-ASM_OBJ := $(ASM_SRC:S=o)
-C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
-+C_OBJ = $(OBJECTS)
++C_OBJ := $(OBJECTS)
$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
@@ Makefile: missing_compdb_dir =
%.s: %.c GIT-CFLAGS FORCE
$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
+ ## block-sha1/sha1.c ##
+@@
+ * try to do the silly "optimize away loads" part because it won't
+ * see what the value will be).
+ *
+- * Ben Herrenschmidt reports that on PPC, the C version comes close
+- * to the optimized asm with this (ie on PPC you don't want that
+- * 'volatile', since there are lots of registers).
+- *
+ * On ARM we get the best code generation by forcing a full memory barrier
+ * between each SHA_ROUND, otherwise gcc happily get wild with spilling and
+ * the stack frame size simply explode and performance goes down the drain.
+
## configure.ac ##
@@ configure.ac: AC_MSG_NOTICE([CHECKS for site configuration])
# tests. These tests take up a significant amount of the total test time
-: ----------- > 2: cb3bc8b5029 Makefile: use $(OBJECTS) instead of $(C_OBJ)
--
2.37.3.1406.g184357183a6
next prev parent reply other threads:[~2022-08-31 9:19 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-12 8:56 SHA1dc on mac Mike Hommey
2020-02-12 16:46 ` Eric Sunshine
2020-02-12 22:31 ` Mike Hommey
2020-02-12 22:40 ` Junio C Hamano
2020-02-23 22:37 ` [PATCH] Remove non-SHA1dc sha1 implementations Mike Hommey
2020-02-24 4:47 ` Jeff King
2020-02-24 4:52 ` Jeff King
2022-03-19 1:02 ` [PATCH] ppc: remove custom SHA-1 implementation Ævar Arnfjörð Bjarmason
2022-03-21 16:39 ` Junio C Hamano
2022-03-21 17:06 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-03-21 21:19 ` brian m. carlson
2022-08-31 9:18 ` Ævar Arnfjörð Bjarmason [this message]
2022-08-31 9:18 ` [PATCH v3 1/2] Makefile + hash.h: remove PPC_SHA1 implementation Ævar Arnfjörð Bjarmason
2022-08-31 9:18 ` [PATCH v3 2/2] Makefile: use $(OBJECTS) instead of $(C_OBJ) Ævar Arnfjörð Bjarmason
2022-08-31 21:44 ` Junio C Hamano
2022-09-01 14:52 ` Ævar Arnfjörð Bjarmason
2022-09-01 15:48 ` Junio C Hamano
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v3-0.2-00000000000-20220831T090744Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=carenas@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=mh@glandium.org \
--cc=sandals@crustytoothpaste.net \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).