Re: [PATCH] ppc: remove custom SHA-1 implementation

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2022, 16:39 UTC
Message-ID: <xmqq5yo72rbm.fsf@gitster.g>
In-Reply-To: <patch-1.1-05dcdca3877-20220319T005952Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 5 quoted lines

> 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.

Yuck.

Show 22 quoted lines

> diff --git a/Makefile b/Makefile
> index 70f0a004e75..965e51f773e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -155,9 +155,6 @@ include shared.mak
>  # Define BLK_SHA1 environment variable to make use of the bundled
>  # optimized C SHA1 routine.
>  #
> -# Define PPC_SHA1 environment variable when running make to make use of
> -# a bundled SHA1 routine optimized for PowerPC.
> -#
>  # 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.
> @@ -1770,14 +1767,14 @@ ifdef OPENSSL_SHA1
>  	EXTLIBS += $(LIB_4_CRYPTO)
>  	BASIC_CFLAGS += -DSHA1_OPENSSL
>  else
> +ifdef PPC_SHA1
> +$(error PPC_SHA1 has been removed! You should almost definitely remove that \
> +knob and use the DC_SHA1 default! See INSTALL for more information)
> +endif

"use the DC_SHA1 default"?

	PPC_SHA1 support is being removed.  Use DC_SHA1 instead, 
	which is the default.

I am wondering if we can make only these four lines the first step of remova, without doing anything else. It would give us a good feel on how many users we may be inconveniencing (not necessarily hurting, as switching to DC_SHA1 would be a good move) by the removal.

Show 8 quoted lines

> @@ -2509,6 +2505,11 @@ OBJECTS += $(SCALAR_OBJECTS)
>  .PHONY: objects
>  objects: $(OBJECTS)
>  
> +# Derived from $(OBJECTS)
> +OBJECTS_C = $(OBJECTS:%.o=%.c)
> +OBJECTS_S = $(OBJECTS:%.o=%.s)
> +OBJECTS_SP = $(OBJECTS:%.o=%.sp)

Usually we build objects from sources, so "derived from" is a puzzling way to call them. I understand we are deriving a list from another list, but it still feels confusing (see below for ASM_OBJ).

This seems to have nothing to do with "we no longer have *.S files" and looks more like "now we have excuse to touch Makefile, make random changes that look subjectively good to me, burying them in the noise so that we can sneak them in without justifying them much in the proposed log message".

Show 10 quoted lines

>  dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
>  dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
>  
> @@ -2540,13 +2541,7 @@ missing_compdb_dir =
>  compdb_args =
>  endif
>  
> -ASM_SRC := $(wildcard $(OBJECTS:o=S))
> -ASM_OBJ := $(ASM_SRC:S=o)
> -C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))

I tend to agree with this patch that these three lines are ugly in multiple ways.

It's a confusing construct. The list of OBJECTS is used as the single source of truth and others are derived by filtering the list and futzing the suffix of the resulting subset of elements; it makes me wonder if it should be the other way around (i.e. we have a list of source files in various languages, all get turned into objects, rather than we have a list of object files, and we see if corresponding source file in possible languages, if any, exists on disk). It is pleasing that we can see them go.

Unfortunately the new "Derived from $(OBJECTS) lists are in the same spirit as used by ASM_SRC we are removing here, aren't they?

Show 5 quoted lines

> -$(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) $<
> -$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
> +$(OBJECTS): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
>  	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<

Now we deal only with *.c sources and all objects come from them, and the action is the same as the old C_OBJ rule, naturally.

>  %.s: %.c GIT-CFLAGS FORCE

This is the only remaining rule regarding assembly and it is the target to generate for debugging, not even used as a source to create object files. Do we need OBJECTS_S defined above? I somehow doubt it. FWIW, I do not see the need for OBJECTS_SP or OBJECTS_C, either. E.g.

Show 6 quoted lines

> @@ -2692,7 +2687,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
>  	--keyword=gettextln --keyword=eval_gettextln
>  XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
>  	--keyword=__ --keyword=N__ --keyword="__n:1,2"
> -LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> +LOCALIZED_C = $(OBJECTS_C) $(LIB_H) $(GENERATED_H)

Shouldn't it be sufficient to use %(OBJECTS:o=c) here, i.e. equating the old C_OBJ with OBJECTS, now we know all objects come from C? The same question for SP_OBJ below, but I won't repeat.