Re: [lto] PATCH: add LTRANS driver

[Ralf Wildenhues <Ralf dot Wildenhues at gmx dot de>]

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