This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [lto] PATCH: add LTRANS driver
- From: Ralf Wildenhues <Ralf dot Wildenhues at gmx dot de>
- To: Ollie Wild <aaw at google dot com>
- Cc: gcc-patches <gcc-patches at gcc dot gnu dot org>, Diego Novillo <dnovillo at google dot com>, Doug Kwan (éæå) <dougkwan at google dot com>
- Date: Tue, 23 Sep 2008 23:07:06 +0200
- Subject: Re: [lto] PATCH: add LTRANS driver
- References: <65dd6fd50809231235g4be56d06v6ff7bd93a417036b@mail.gmail.com>
Hello Ollie,
This is not an approval, merely a quick review of the build bits of your
patch:
* Ollie Wild wrote on Tue, Sep 23, 2008 at 09:35:05PM CEST:
> --- a/gcc/lto/Make-lang.in
> +++ b/gcc/lto/Make-lang.in
> @@ -94,3 +99,6 @@ lto/lto-elf.o: lto/lto-elf.c $(CONFIG_H) coretypes.h $(SYSTEM_H) \
> lto/lto-symtab.o: lto/lto-symtab.c lto-tree-in.h $(CONFIG_H) coretypes.h \
> $(SYSTEM_H) toplev.h $(LTO_H) lto/lto-tree.h $(GGC_H) $(LAMBDA_H) \
> $(GIMPLE_H)
> +
> +lto/ltrans-driver: lto/ltrans-driver.in
> + CONFIG_FILES=lto/ltrans-driver CONFIG_HEADERS= ./config.status
Ugh, using that old-fashioned way. Why not
./config.status lto/ltrans-driver
after adding
AC_CONFIG_FILES([lto/ltrans-driver])
to configure.ac?
> --- /dev/null
> +++ b/gcc/lto/ltrans-driver.in
> +# This is the default ltrans_driver for LTO. It takes a list of files
> +# output during WPA mode and expects the CC and CFLAGS environment variables
> +# to be set. It generates a temporary Makefile, and uses it to convert .o
> +# files to .ltrans.o files. Other environment variables (e.g. MAKEFLAGS) can
> +# be used to control the behavior of make.
Why is $MAKE not used?
> +# Initialize TMPDIR if it isn't already.
> +if [ -z "$TMPDIR" ]; then
> + TMPDIR=/tmp
> +else
> + if [ "`echo $TMPDIR | grep '/$'`" != "" ]; then
case $TMPDIR in */) ... ;; esac
is faster and less buggy. Not sure why you even need this at all
> + TMPDIR="`echo $TMPDIR | sed -e 's,/$,,'`"
Underquoted $TMPDIR. If you quote it (as above you should just remove
this section), then remove the outer "" quotes (they are unnecessary and
nesting "" and `` and "" is unportable).
> + fi
> +fi
> +# Create a temporary Makefile. This is intended to be secure.
> +if [ @have_mktemp_command@ = yes ]; then
> + MAKEFILE=$(mktemp $TMPDIR/Makefile.lto.XXXXXX)
Please use the code from the mktemp section here:
<http://www.gnu.org/software/autoconf/manual/html_node/Limitations-of-Usual-Tools.html>
to create a temporary file, with adaptation of the file name prefix.
You can probably just delete all code above then.
FWIW, you underquote $TMPDIR here. Also $() is not portable to Solaris
10 /bin/sh, so please use `..`.
> + REMOVE_TEMP="rm -f $MAKEFILE"
> +else
> + TEMPD="$TMPDIR/ltrans$$"
> + MAKEFILE=$TEMPD/Makefile.lto.$$
FWIW there is no gain in using $$ twice in a path name.
> + (umask 077 && mkdir $TEMPD) || exit 1
> + touch $MAKEFILE
> + REMOVE_TEMP="rm -rf $TEMPD"
Sure you don't want to use a trap to clean up?
> +fi
> +
> +echo "LTRANS_FLAGS = -xlto -fltrans" >> $MAKEFILE
> +
> +INPUTLIST="$@"
> +for INPUT in $INPUTLIST; do
FWIW, these two lines are easier written as
for INPUT
do
(yes, with the newline). I kind of wonder why you use all-caps for
all the variables.
> + OUTPUT="$(basename $INPUT .o).ltrans.o"
See above: $(). A sed script is probably more portable than basename
(but here I'm not sure whether there is a problem in practice).
> + OUTPUTLIST="$OUTPUTLIST $OUTPUT"
> +
> + echo "$OUTPUT: $INPUT" >> $MAKEFILE
> + echo "\t\$(CC) \$(CFLAGS) \$(LTRANS_FLAGS) -o $OUTPUT $INPUT" >> $MAKEFILE
echo '\t' is not portable. Please enter a literal TAB here.
> + echo >> $MAKEFILE
> +done
> +
> +echo "all: $OUTPUTLIST" >> $MAKEFILE
> +
> +make -f $MAKEFILE all
See above: ${MAKE-make}
> +$REMOVE_TEMP
Cheers,
Ralf