Re: [PATCH] Move bench target into benchtests

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/09/2013 12:45 AM, Siddhesh Poyarekar wrote:
> On Mon, Apr 08, 2013 at 03:09:56PM -0700, Roland McGrath wrote:
>> Why is any of this stuff in Rules at all?  It appears to be relevant only
>> to bench/Makefile and so it should be there.  Is there some intent to
>> support the 'bench' variable in other subdir makefiles?
> 
> Right, there is no reason to have it in the root directory.  Here's a
> patch to move the rules within benchtests.  Looks OK?
> 
> Siddhesh
> 
> 	* Rules (bench): Move target definition...
> 	* benchtests/Rules: ... into this new file.
> 	* benchtests/Makefile: Adjust.

Looks good to me. One nit below.

> diff --git a/Rules b/Rules
> index d4a0027..86a0520 100644
> --- a/Rules
> +++ b/Rules
> @@ -189,36 +189,6 @@ $(objpfx)%.out: /dev/null $(objpfx)%	# Make it 2nd arg for canned sequence.
>  
>  endif	# tests
>  
> -# Build and run benchmark programs.
> -binaries-bench := $(addprefix $(objpfx)bench-,$(bench))
> -
> -run-bench = $(test-wrapper-env) \
> -	    GCONV_PATH=$(common-objpfx)iconvdata LC_ALL=C \
> -	    $($*-ENV) $(run-via-rtld-prefix) $${run}
> -
> -bench: $(binaries-bench)
> -	for run in $^; do \
> -	  echo "Running $${run}"; \
> -	  eval $(run-bench) >>  $(objpfx)bench.out-tmp; \
> -	done; \
> -	if [ -f $(objpfx)bench.out ]; then \
> -	  mv -f $(objpfx)bench.out $(objpfx)bench.out.old; \
> -	fi; \
> -	mv -f $(objpfx)bench.out-tmp $(objpfx)bench.out
> -
> -$(binaries-bench): %: %.o \
> -  $(sort $(filter $(common-objpfx)lib%,$(link-libc))) \
> -  $(addprefix $(csu-objpfx),start.o) $(+preinit) $(+postinit)
> -	$(+link)
> -
> -$(objpfx)bench-%.c: %-inputs bench-skeleton.c
> -	{ if [ -n "$($*-INCLUDE)" ]; then \
> -	  cat $($*-INCLUDE); \
> -	fi; \
> -	$(..)scripts/bench.pl $(patsubst %-inputs,%,$<) \
> -	  $($*-ITER) $($*-ARGLIST) $($*-RET); } > $@-tmp
> -	mv -f $@-tmp $@
> -
>  
>  .PHONY: distclean realclean subdir_distclean subdir_realclean \
>  	subdir_clean subdir_mostlyclean subdir_testclean
> diff --git a/benchtests/Makefile b/benchtests/Makefile
> index a6a9299..d3d5253 100644
> --- a/benchtests/Makefile
> +++ b/benchtests/Makefile
> @@ -105,4 +105,4 @@ slowatan-INCLUDE = slowatan.c
>  LDFLAGS-bench-slowatan = -lm
>  
>  include ../Makeconfig
> -include ../Rules
> +include Rules
> diff --git a/benchtests/Rules b/benchtests/Rules
> new file mode 100644
> index 0000000..4715084
> --- /dev/null
> +++ b/benchtests/Rules
> @@ -0,0 +1,53 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +# This file is part of the GNU C Library.
> +
> +# The GNU C Library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +
> +# The GNU C Library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with the GNU C Library; if not, see
> +# <http://www.gnu.org/licenses/>.
> +
> +# Rules to build and run benchmark programs.
> +
> +include ../Rules
> +
> +binaries-bench := $(addprefix $(objpfx)bench-,$(bench))
> +
> +run-bench = $(test-wrapper-env) \
> +	    GCONV_PATH=$(common-objpfx)iconvdata LC_ALL=C \
> +	    $($*-ENV) $(run-via-rtld-prefix) $${run}
> +
> +bench: $(binaries-bench)
> +	for run in $^; do \
> +	  echo "Running $${run}"; \
> +	  eval $(run-bench) >>  $(objpfx)bench.out-tmp; \

Do you need `eval' here? Schwab caught this in my implementation also,
and I didn't need it because all the variables were resolved by the time
the command is evaluated.

> +	done; \
> +	if [ -f $(objpfx)bench.out ]; then \
> +	  mv -f $(objpfx)bench.out $(objpfx)bench.out.old; \
> +	fi; \
> +	mv -f $(objpfx)bench.out-tmp $(objpfx)bench.out
> +
> +$(binaries-bench): %: %.o \
> +  $(sort $(filter $(common-objpfx)lib%,$(link-libc))) \
> +  $(addprefix $(csu-objpfx),start.o) $(+preinit) $(+postinit)
> +	$(+link)
> +
> +$(objpfx)bench-%.c: %-inputs bench-skeleton.c
> +	{ if [ -n "$($*-INCLUDE)" ]; then \
> +	  cat $($*-INCLUDE); \
> +	fi; \
> +	$(..)scripts/bench.pl $(patsubst %-inputs,%,$<) \
> +	  $($*-ITER) $($*-ARGLIST) $($*-RET); } > $@-tmp
> +	mv -f $@-tmp $@
> +
> +# Local Variables:
> +# mode: makefile
> +# End:
> 

Cheers,
Carlos.