Re: [PATCH] [gdb/contrib] Make gdb-add-index.sh shellcheck-clean

[Tom de Vries <tdevries@suse.de>]

On 9/22/26 10:13 PM, Keith Seitz wrote:
> Hi,
> 
> Thank you for doing this.
> 
> On 9/15/26 10:14 AM, Tom de Vries wrote:
>> Make gdb-add-index.sh shellcheck-clean:
>> - drop x in "xa" != "xb" comparison
>> - eliminate tmp_files and use the args array instead
>> - drop dwz_file=$(echo $dwz_file) and strip leading spaces using
>>    sed instead
>> ---
>>   gdb/contrib/gdb-add-index.sh | 19 ++++++++++++-------
>>   gdb/contrib/shellcheck.sh    |  4 ----
>>   2 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
>> index 4a30c5c5536..e1a37f2cd4c 100755
>> --- a/gdb/contrib/gdb-add-index.sh
>> +++ b/gdb/contrib/gdb-add-index.sh
>> @@ -82,7 +82,7 @@ case ${opt} in
>>       ;;
>>   esac
>>   # Break from loop if the first character of OPT is not '-'.
>> -[ "x$(printf %.1s "$opt")" != "x-" ]
>> +[ "$(printf %.1s "$opt")" != "-" ]
>>   do
>>       shift
>>   done
>> @@ -93,6 +93,7 @@ if test $# != 1; then
>>   fi
>>   file="$1"
>> +set --
> 
> This unsets the positional parameters, clearing $1, but this is
> subsequently used if readlink fails:
> 
>>   if test -L "$file"; then
>>       if ! command -v readlink >/dev/null 2>&1; then
>          echo "$myname: 'readlink' missing.  Failed to follow symlink 
> $1." 1>&2
>          exit 1
>      fi
> 
> Does this need to change to "$file"?
> 

Indeed, thanks for catching that.

>> @@ -135,8 +136,7 @@ if $READELF -S "$file" | grep -q " 
>> \.gnu_debugaltlink "; then
>>       dwz_file=$($READELF --string-dump=.gnu_debugaltlink "$file" \
>>              | grep -A1  "'\.gnu_debugaltlink':" \
>>              | tail -n +2 \
>> -           | sed 's/.*]//')
>> -    dwz_file=$(echo $dwz_file)
>> +           | sed 's/.*] *//')
>>       if $READELF -S "$dwz_file" | grep -E -q " \.(gdb_index| 
>> debug_names) "; then
>>       # Already has an index, skip it.
>>       dwz_file=""
>> @@ -154,19 +154,24 @@ set_files ()
>>       debugstrerr="${fpath}.debug_str.err"
>>   }
>> -tmp_files=
>>   for f in "$file" "$dwz_file"; do
>>       if [ "$f" = "" ]; then
>>       continue
>>       fi
>>       set_files "$f"
>> -    tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge 
>> $debugstrerr"
>> +    set -- \
>> +    "$@" \
>> +    "$index4" \
>> +    "$index5" \
>> +    "$debugstr" \
>> +    "$debugstrmerge" \
>> +    "$debugstrerr"
>>   done
>> -rm -f $tmp_files
>> +rm -f "$@"
>>   # Ensure intermediate index file is removed when we exit.
>> -trap "rm -f $tmp_files" 0
>> +trap 'rm -f "$@"' 0
> 
> I'm not sure this is safe.
> 
> The "$@" in the trap is expanded when the trap runs, using whatever
> positional parameters are active in that context. If that happens when
> exit is called from a function (as in handle_file on objcopy failure),
> bash substitutes the function's arguments instead of the temp-file list,
> erasing the real binary and leaving the temps behind. [On Fedora,
> /bin/sh --> /bin/bash.]
> 
> Demonstration:
> $ cat > trap-test.sh << 'EOF'
> #! /bin/sh
> # These are our temporary files...
> set -- /tmp/index.tmp /tmp/debugstr.tmp
> touch "$@"
> 
> trap 'echo "trap removes: $*"; rm -f "$@"' EXIT
> 
> handle_file () {
>      echo "in handle_file, \$1=$1"
>      exit 1   # like gdb-add-index.sh on objcopy failure
> }
> 
> handle_file /tmp/important-binary
> EOF
> $ touch /tmp/important-binary; # for clarity
> $ bash trap-test.sh
> in handle_file, $1=/tmp/important-binary
> trap removes: /tmp/important-binary
> 

I see, I didn't take that into account.

I can fix this by doing:
...
trap '{ 

     skip=true 

     for arg in "$@"; do 

         if [ "$arg" = "--" ]; then 

             skip=false 

             continue 

         fi 

         if $skip; then 

             continue 

         fi 

         rm -f "$arg" 

     done 

}' 0
...

and doing:
...
andle_file "$file" -- "$@"
if [ "$dwz_file" != "" ]; then
     handle_file "$dwz_file" -- "$@"
fi
...
and like wise here:
...
     set -- \
	"--" \
         "$@" \
         "$index4" \
         "$index5" \
         "$debugstr" \
         "$debugstrmerge" \
	"$debugstrerr"
...

But it all feels a bit convoluted.

I'll try to come up with something else.

Thanks for catching this.
- Tom

> Keith
>