Re: [PATCH v3 1/5] cc-with-tweaks.sh: Use gdb-add-index.sh

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Jan,

On 2017-06-19 22:55, Jan Kratochvil wrote:
> Hi,
> 
> currently contrib/cc-with-tweaks.sh is calling for its -i option 
> objcopy itself
> instead of using contrib/gdb-add-index.sh which does the same.
> 
> With DWARF-5 .debug_names the commands are more complicated (as now 
> also
> .debug_str needs to be modified) and so I have decided to rather reuse
> contrib/gdb-add-index.sh instead of duplicating its code
> in contrib/cc-with-tweaks.sh.
> 
> The problem is when no index is produced whether 
> contrib/cc-with-tweaks.sh
> should fail or not.  As originally contrib/cc-with-tweaks.sh was more 
> quiet
> (=successful) than contrib/gdb-add-index.sh and so after this patch the
> testsuite runs with an index would "regress".  I have tried to keep the
> behavior unchanged.  Some cases still error with:
> 	Ada is not currently supported by the index
> But some cases (such as some trivial gdb.dwarf2/ testcases with no 
> DWARF data
> to index) produce no index while the testcases still PASS now instead 
> of:
> 	-PASS: gdb.arch/i386-bp_permanent.exp: stack pointer value matches
> 	+gdb compile failed, gdb-add-index.sh: No index was created for
> /quadgdb/testsuite.unix.-m64/outputs/gdb.arch/i386-bp_permanent/i386-bp_permanent
> 	+gdb-add-index.sh: [Was there no debuginfo? Was there already an 
> index?]
> 	+UNTESTED: gdb.arch/i386-bp_permanent.exp: failed to compile
> 
> 
> Jan
> 
> 
> gdb/ChangeLog
> 2017-05-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* contrib/cc-with-tweaks.sh (t, GDB_ADD_INDEX): New variables.
> 	<$want_index>: Call $GDB_ADD_INDEX.
> ---
>  gdb/contrib/cc-with-tweaks.sh |   40 
> ++++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/contrib/cc-with-tweaks.sh 
> b/gdb/contrib/cc-with-tweaks.sh
> index 7d39c00..c5a8088 100755
> --- a/gdb/contrib/cc-with-tweaks.sh
> +++ b/gdb/contrib/cc-with-tweaks.sh
> @@ -74,6 +74,8 @@ DWP=${DWP:-dwp}
>  have_link=unknown
>  next_is_output_file=no
>  output_file=a.out
> +t=/tmp/cc-with-tweaks.$$
> +rm -f $t
> 
>  want_index=false
>  want_dwz=false
> @@ -93,6 +95,25 @@ while [ $# -gt 0 ]; do
>      shift
>  done
> 
> +if [ "$want_index" = true ]; then
> +  if [ -z "$GDB_ADD_INDEX" ]
> +  then

Nit: use the same style ("then" on same line or next line) for all ifs.

> +      if [ -f ./contrib/gdb-add-index.sh ]
> +      then
> +	  GDB_ADD_INDEX="./contrib/gdb-add-index.sh"
> +      elif [ -f ../contrib/gdb-add-index.sh ]
> +      then
> +	  GDB_ADD_INDEX="../contrib/gdb-add-index.sh"
> +      elif [ -f ../../contrib/gdb-add-index.sh ]
> +      then
> +	  GDB_ADD_INDEX="../../contrib/gdb-add-index.sh"
> +      else
> +	  echo "$myname: unable to find usable contrib/gdb-add-index.sh" >&2
> +	  exit 1
> +      fi
> +  fi
> +fi

This strategy doesn't work for out of tree builds.  Perhaps cooking up 
something based on $0 would be better?  I see that the GDB variable is 
auto-detected the same way, but the gdb binary is found in the build 
directory, so it works for the tests, whereas gdb-add-index.sh is in the 
source directory.

I think it would still be nice to improve how GDB is auto-detected (in 
another patch), because one currently can't call the cc-with-tweaks.sh 
script from anywhere:

$ cd / && /home/emaisin/src/binutils-gdb/gdb/contrib/cc-with-tweaks.sh
cc-with-tweaks.sh: unable to find usable gdb

> +
>  for arg in "$@"
>  do
>      if [ "$next_is_output_file" = "yes" ]
> @@ -152,20 +173,11 @@ if [ "$want_objcopy_compress" = true ]; then
>  fi
> 
>  if [ "$want_index" = true ]; then
> -    $GDB --batch-silent -nx -ex "set auto-load no" -ex "file
> $output_file" -ex "save gdb-index $output_dir"
> -    rc=$?
> -    [ $rc != 0 ] && exit $rc
> -
> -    # GDB might not always create an index.  Cope.
> -    if [ -f "$index_file" ]
> -    then
> -	$OBJCOPY --add-section .gdb_index="$index_file" \
> -	    --set-section-flags .gdb_index=readonly \
> -	    "$output_file" "$output_file"
> -	rc=$?
> -    else
> -	rc=0
> -    fi
> +    # Filter out these messages which would stop dejagnu testcase run:
> +    # echo "$myname: No index was created for $file" 1>&2
> +    # echo "$myname: [Was there no debuginfo? Was there already an
> index?]" 1>&2
> +    GDB=$GDB $GDB_ADD_INDEX "$output_file" 2>&1|grep -v
> "^${GDB_ADD_INDEX##*/}: " >&2
> +    rc=${PIPESTATUS[0]}

Is PIPESTATUS bash specific?  If so, we should probably change the 
interpreter to

#!/usr/bin/env bash

The CC/CXX_FOR_TARGET variables in cc-with-tweaks.exp also use /bin/sh 
explicitly, I think it those can just be removed.

Thanks,

Simon