[PATCH 0/2] shell fixes for gcore, gdb-add-index

[PATCH 0/2] shell fixes for gcore, gdb-add-index

[Sam James]

As suggested by Simon Marchi <simark@simark.ca>.

Sam James (2):
  gcore: improve shell use
  gdb-add-index: fix shellcheck warnings

 gdb/contrib/gdb-add-index.sh |  7 +++----
 gdb/gcore-1.in               | 22 +++++++++++-----------
 2 files changed, 14 insertions(+), 15 deletions(-)

-- 
2.49.0

[PATCH 1/2] gcore: improve shell use

[Sam James]

* Use bash tests, i.e. '[[' and ']]', instead of POSIX '[' and ']' or
  'test' which have their own pitfalls, given gcore has a bash shebang already.

* Use $() subshell syntax rather than `backticks`.
---
 gdb/gcore-1.in | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/gcore-1.in b/gdb/gcore-1.in
index c0979a51db3..edd807b86dc 100644
--- a/gdb/gcore-1.in
+++ b/gdb/gcore-1.in
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-#   Copyright (C) 2003-2024 Free Software Foundation, Inc.
+#   Copyright (C) 2003-2025 Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -64,7 +64,7 @@ function print_version() {
 }
 
 while getopts vhao:d:-: OPT; do
-    if [ "$OPT" = "-" ]; then
+    if [[ "$OPT" = "-" ]]; then
 	OPT="${OPTARG%%=*}"
 	OPTARG="${OPTARG#'$OPT'}"
 	OPTARG="${OPTARG#=}"
@@ -111,7 +111,7 @@ done
 
 shift $((OPTIND-1))
 
-if [ "$#" -eq "0" ]
+if [[ "$#" -eq "0" ]]
 then
     print_usage 1>&2
     exit 1
@@ -119,19 +119,19 @@ fi
 
 # Attempt to fetch the absolute path to the gcore script that was
 # called.
-binary_path=`dirname "$0"`
+binary_path=$(dirname "$0")
 
-if test "x$binary_path" = x. ; then
+if [[ $binary_path == x. ]] ; then
   # We got "." back as a path.  This means the user executed
   # the gcore script locally (i.e. ./gcore) or called the
   # script via a shell interpreter (i.e. sh gcore).
-  binary_basename=`basename "$0"`
+  binary_basename=$(basename "$0")
 
   # If the gcore script was called like "sh gcore" and the script
   # lives in the current directory, "which" will not give us "gcore".
   # So first we check if the script is in the current directory
   # before using the output of "which".
-  if test -f "$binary_basename" ; then
+  if [[ -f "$binary_basename" ]]; then
     # We have a local gcore script in ".".  This covers the case of
     # doing "./gcore" or "sh gcore".
     binary_path="."
@@ -139,14 +139,14 @@ if test "x$binary_path" = x. ; then
     # The gcore script was not found in ".", which means the script
     # was called from somewhere else in $PATH by "sh gcore".
     # Extract the correct path now.
-    binary_path_from_env=`which "$0"`
-    binary_path=`dirname "$binary_path_from_env"`
+    binary_path_from_env=$(which "$0")
+    binary_path=$(dirname "$binary_path_from_env")
   fi
 fi
 
 # Check if the GDB binary is in the expected path.  If not, just
 # quit with a message.
-if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
+if [[ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]]; then
   echo "gcore: GDB binary (${binary_path}/@GDB_TRANSFORM_NAME@) not found"
   exit 1
 fi
@@ -166,7 +166,7 @@ do
 	    "${dump_all_cmds[@]}" \
 	    -ex "attach $pid" -ex "gcore $prefix.$pid" -ex detach -ex quit
 
-	if [ -r "$prefix.$pid" ] ; then
+	if [[ -r "$prefix.$pid" ]] ; then
 	    rc=0
 	else
 	    echo "@GCORE_TRANSFORM_NAME@: failed to create $prefix.$pid"
-- 
2.49.0

Re: [PATCH 1/2] gcore: improve shell use

[Keith Seitz]

Hi,

On 3/28/25 11:58 AM, Sam James wrote:
> * Use bash tests, i.e. '[[' and ']]', instead of POSIX '[' and ']' or
>    'test' which have their own pitfalls, given gcore has a bash shebang already.
> 
> * Use $() subshell syntax rather than `backticks`.

Nice. I didn't actually know that '[['/']]' was preferred. I'll have
to remember that.

> ---
>   gdb/gcore-1.in | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/gcore-1.in b/gdb/gcore-1.in
> index c0979a51db3..edd807b86dc 100644
> --- a/gdb/gcore-1.in
> +++ b/gdb/gcore-1.in
> @@ -139,14 +139,14 @@ if test "x$binary_path" = x. ; then
>       # The gcore script was not found in ".", which means the script
>       # was called from somewhere else in $PATH by "sh gcore".
>       # Extract the correct path now.
> -    binary_path_from_env=`which "$0"`
> -    binary_path=`dirname "$binary_path_from_env"`
> +    binary_path_from_env=$(which "$0")
> +    binary_path=$(dirname "$binary_path_from_env")

This uses 'which', and we've (downstream distros) run into issues
with this and package dependencies. Since this strictly bash,
can we use "command -v" here instead?

Thank you for doing this.

Reviewed-By: Keith Seitz <keiths@redhat.com>

Keith

>     fi
>   fi
>   
>   # Check if the GDB binary is in the expected path.  If not, just
>   # quit with a message.
> -if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then
> +if [[ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]]; then
>     echo "gcore: GDB binary (${binary_path}/@GDB_TRANSFORM_NAME@) not found"
>     exit 1
>   fi

Re: [PATCH 1/2] gcore: improve shell use

[Simon Marchi]

On 3/29/25 7:42 PM, Keith Seitz wrote:
> Hi,
> 
> On 3/28/25 11:58 AM, Sam James wrote:
>> * Use bash tests, i.e. '[[' and ']]', instead of POSIX '[' and ']' or
>>    'test' which have their own pitfalls, given gcore has a bash shebang already.
>>
>> * Use $() subshell syntax rather than `backticks`.
> 
> Nice. I didn't actually know that '[['/']]' was preferred. I'll have
> to remember that.
> 
>> ---
>>   gdb/gcore-1.in | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/gcore-1.in b/gdb/gcore-1.in
>> index c0979a51db3..edd807b86dc 100644
>> --- a/gdb/gcore-1.in
>> +++ b/gdb/gcore-1.in
>> @@ -139,14 +139,14 @@ if test "x$binary_path" = x. ; then
>>       # The gcore script was not found in ".", which means the script
>>       # was called from somewhere else in $PATH by "sh gcore".
>>       # Extract the correct path now.
>> -    binary_path_from_env=`which "$0"`
>> -    binary_path=`dirname "$binary_path_from_env"`
>> +    binary_path_from_env=$(which "$0")
>> +    binary_path=$(dirname "$binary_path_from_env")
> 
> This uses 'which', and we've (downstream distros) run into issues
> with this and package dependencies. Since this strictly bash,
> can we use "command -v" here instead?

Can you clarify what kind of issues?  Do you mean that `which` may not
be available?

shellcheck points out this:

In gcore line 69:
        OPTARG="${OPTARG#'$OPT'}"
                         ^----^ SC2016 (info): Expressions don't expand in single quotes, use double quotes for that.

I actually can't figure out what this "if" does:

    if [[ "$OPT" = "-" ]]; then
	OPT="${OPTARG%%=*}"
	OPTARG="${OPTARG#'$OPT'}"
	OPTARG="${OPTARG#=}"
    fi

Any idea?

Simon

Re: [PATCH 1/2] gcore: improve shell use

[Andreas Schwab]

On Mär 31 2025, Simon Marchi wrote:

> I actually can't figure out what this "if" does:
>
>     if [[ "$OPT" = "-" ]]; then
> 	OPT="${OPTARG%%=*}"
> 	OPTARG="${OPTARG#'$OPT'}"
> 	OPTARG="${OPTARG#=}"
>     fi
>
> Any idea?

I think this is supposed to handle the long options --help and
--version.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/2] gcore: improve shell use

[Simon Marchi]

On 3/31/25 2:44 PM, Andreas Schwab wrote:
> On Mär 31 2025, Simon Marchi wrote:
> 
>> I actually can't figure out what this "if" does:
>>
>>     if [[ "$OPT" = "-" ]]; then
>> 	OPT="${OPTARG%%=*}"
>> 	OPTARG="${OPTARG#'$OPT'}"
>> 	OPTARG="${OPTARG#=}"
>>     fi
>>
>> Any idea?
> 
> I think this is supposed to handle the long options --help and
> --version.

Oh, right, looks like a nice hack to support long options, which getopts
doesn't appear to support officially.

I think that indeed the line doesn't work as intended then.  I hacked
gcore to print OPT and OPTARG.  I get:

    $ ./gcore --version
    OPT: version
    OPTARG: version

OPTARG should be empty here.  If I change

    OPTARG="${OPTARG#'$OPT'}"

to

    OPTARG="${OPTARG#"$OPT"}"

then I get:

    $ ./gcore --version
    OPT: version                                                                                             │
    OPTARG:

There is not long option that uses OPTARG, so this bug has no impact,
but we might as well fix it in this patch.

Simon

Re: [PATCH 1/2] gcore: improve shell use

[Keith Seitz]

On 3/31/25 11:54 AM, Simon Marchi wrote:
> I think that indeed the line doesn't work as intended then.  I hacked
> gcore to print OPT and OPTARG.  I get:
> 
>      $ ./gcore --version
>      OPT: version
>      OPTARG: version
> 
> OPTARG should be empty here.  If I change
> 
>      OPTARG="${OPTARG#'$OPT'}"
> 
> to
> 
>      OPTARG="${OPTARG#"$OPT"}"
> 
> then I get:
> 
>      $ ./gcore --version
>      OPT: version                                                                                             │
>      OPTARG:
> 
> There is not long option that uses OPTARG, so this bug has no impact,
> but we might as well fix it in this patch.

Yes, I agree. My darn fingers still get confused between python quoting
and shell quoting.

Thank you for pursuing this,
Keith

Re: [PATCH 1/2] gcore: improve shell use

[Tom Tromey]

>>>>> "Sam" == Sam James <sam@gentoo.org> writes:

Sam> * Use bash tests, i.e. '[[' and ']]', instead of POSIX '[' and ']' or
Sam>   'test' which have their own pitfalls, given gcore has a bash shebang already.

Do we want this to require bash or should it instead just use sh?

Tom

Re: [PATCH 1/2] gcore: improve shell use

[Simon Marchi]

On 4/1/25 9:39 AM, Tom Tromey wrote:
>>>>>> "Sam" == Sam James <sam@gentoo.org> writes:
> 
> Sam> * Use bash tests, i.e. '[[' and ']]', instead of POSIX '[' and ']' or
> Sam>   'test' which have their own pitfalls, given gcore has a bash shebang already.
> 
> Do we want this to require bash or should it instead just use sh?
> 
> Tom

gcore already requires bash as we speak, which IMO is reasonable.

Simon

Re: [PATCH 1/2] gcore: improve shell use

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 4/1/25 9:39 AM, Tom Tromey wrote:
>>>>>>> "Sam" == Sam James <sam@gentoo.org> writes:
>> 
Sam> * Use bash tests, i.e. '[[' and ']]', instead of POSIX '[' and ']' or
Sam> 'test' which have their own pitfalls, given gcore has a bash shebang already.
>> 
>> Do we want this to require bash or should it instead just use sh?

Simon> gcore already requires bash as we speak, which IMO is reasonable.

I understand that it requires it presently.  I'm asking whether the
script should require bash.

I tend to think requiring bash is an artifact of its history, coming
from a Linux vendor.  But perhaps bash isn't really required and we
could make it portable.

thanks,
Tom

[PATCH 2/2] gdb-add-index: fix shellcheck warnings

[Sam James]

* Drop useless echo
* Add missing quoting to rm argument (and use -- to delimit options)
* Fix quoting w/ trap to avoid expanding $tmp_files immediately
---
 gdb/contrib/gdb-add-index.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
index b299f830373..f727d28aac7 100755
--- a/gdb/contrib/gdb-add-index.sh
+++ b/gdb/contrib/gdb-add-index.sh
@@ -2,7 +2,7 @@
 
 # Add a .gdb_index section to a file.
 
-# Copyright (C) 2010-2024 Free Software Foundation, Inc.
+# Copyright (C) 2010-2025 Free Software Foundation, Inc.
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 3 of the License, or
@@ -136,7 +136,6 @@ if $READELF -S "$file" | grep -q " \.gnu_debugaltlink "; then
 		   | grep -A1  "'\.gnu_debugaltlink':" \
 		   | tail -n +2 \
 		   | sed 's/.*]//')
-    dwz_file=$(echo $dwz_file)
     if $READELF -S "$dwz_file" | grep -E -q " \.(gdb_index|debug_names) "; then
 	# Already has an index, skip it.
 	dwz_file=""
@@ -163,10 +162,10 @@ for f in "$file" "$dwz_file"; do
     tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
 done
 
-rm -f $tmp_files
+rm -f -- "$tmp_files"
 
 # Ensure intermediate index file is removed when we exit.
-trap "rm -f $tmp_files" 0
+trap 'rm -f $tmp_files' 0
 
 $GDB --batch -nx -iex 'set auto-load no' \
     -iex 'set debuginfod enabled off' \
-- 
2.49.0

Re: [PATCH 2/2] gdb-add-index: fix shellcheck warnings

[Keith Seitz]

Hi,

On 3/28/25 11:58 AM, Sam James wrote:
> * Drop useless echo
> * Add missing quoting to rm argument (and use -- to delimit options)
> * Fix quoting w/ trap to avoid expanding $tmp_files immediately

Again, thank you for doing this.

> ---
>   gdb/contrib/gdb-add-index.sh | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
> index b299f830373..f727d28aac7 100755
> --- a/gdb/contrib/gdb-add-index.sh
> +++ b/gdb/contrib/gdb-add-index.sh
> @@ -163,10 +162,10 @@ for f in "$file" "$dwz_file"; do
>       tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
>   done
>   
> -rm -f $tmp_files
> +rm -f -- "$tmp_files"

By preventing expansion, wouldn't this be attempting to
erase a single file whose name is a concatenation of all the
temporary files?

That is,
$ touch a b c
$ tmp="a b c"
$ rm -- "$tmp"
rm: cannot remove 'a b c': No such file or directory

Keith

Re: [PATCH 2/2] gdb-add-index: fix shellcheck warnings

[Simon Marchi]

On 3/29/25 7:42 PM, Keith Seitz wrote:
> Hi,
> 
> On 3/28/25 11:58 AM, Sam James wrote:
>> * Drop useless echo
>> * Add missing quoting to rm argument (and use -- to delimit options)
>> * Fix quoting w/ trap to avoid expanding $tmp_files immediately
> 
> Again, thank you for doing this.
> 
>> ---
>>   gdb/contrib/gdb-add-index.sh | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/contrib/gdb-add-index.sh b/gdb/contrib/gdb-add-index.sh
>> index b299f830373..f727d28aac7 100755
>> --- a/gdb/contrib/gdb-add-index.sh
>> +++ b/gdb/contrib/gdb-add-index.sh
>> @@ -163,10 +162,10 @@ for f in "$file" "$dwz_file"; do
>>       tmp_files="$tmp_files $index4 $index5 $debugstr $debugstrmerge $debugstrerr"
>>   done
>>   -rm -f $tmp_files
>> +rm -f -- "$tmp_files"
> 
> By preventing expansion, wouldn't this be attempting to
> erase a single file whose name is a concatenation of all the
> temporary files?
> 
> That is,
> $ touch a b c
> $ tmp="a b c"
> $ rm -- "$tmp"
> rm: cannot remove 'a b c': No such file or directory
> 
> Keith
> 

Indeed, this looks wrong.  But not quoting it is also a problem if there
are spaces in the paths.  I think a sane way to handle this would be to
require bash and use arrays [1].

Simon

[1] https://www.gnu.org/software/bash/manual/html_node/Arrays.html