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

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

[Tom de Vries]

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 --
 
 if test -L "$file"; then
     if ! command -v readlink >/dev/null 2>&1; then
@@ -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
 
 $GDB --batch -nx -iex 'set auto-load no' \
     -iex 'set debuginfod enabled off' \
diff --git a/gdb/contrib/shellcheck.sh b/gdb/contrib/shellcheck.sh
index 6417d7a871f..9056206ea5f 100755
--- a/gdb/contrib/shellcheck.sh
+++ b/gdb/contrib/shellcheck.sh
@@ -41,10 +41,6 @@ for f in "$@"; do
 	    # Skip generated files.
 	    continue
 	    ;;
-	gdb/contrib/gdb-add-index.sh )
-	    # Skip unclean files.
-	    continue
-	    ;;
 	*)
 	    files=("${files[@]}" "$f")
 	    ;;

base-commit: eec39a2f9250c622d86219cc182c4feaa6b07c50
-- 
2.51.0

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

[Keith Seitz]

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"?

> @@ -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

Keith

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

[Tom de Vries]

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
> 

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

[Tom de Vries]

On 9/23/26 12:27 AM, Tom de Vries wrote:
> I'll try to come up with something else.

I found a solution:
...
+cleanup ()
+{
+    for f in "$file" "$dwz_file"; do
+	if [ "$f" = "" ]; then
+	    continue
+	fi
+	set_files "$f"
+	rm -f \
+	   "$index4" \
+	   "$index5" \
+	   "$debugstr" \
+	   "$debugstrmerge" \
+	   "$debugstrerr"
+    done
+}
...

Rather than collecting the files into a list, and using the list for the 
cleanup, use them directly in a cleanup function.

Submitted as v2 here ( 
https://sourceware.org/pipermail/gdb-patches/2026-September/230559.html ).

Thanks,
- Tom