[PATCH 0/7] Make gdbarch.sh shellcheck-clean

[PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Simon Marchi]

I ran shellcheck on gdbarch.sh and addressed all the warnings.  It
didn't catch anything serious, but I think it's good to have it clean
anyway, so we can catch potential problems in future changes we do to
this file.

Simon Marchi (7):
  gdb: fix shellcheck warnings SC2059 (variables in printf format
    string) in gdbarch.sh
  gdb: fix shellcheck warnings SC2086 (missing double quotes) in
    gdbarch.sh
  gdb: fix shellcheck warnings SC2006 (use $() instead of ``) in
    gdbarch.sh
  gdb: fix shellcheck warnings SC2166 (&& and !! instead of -a and -o)
    in gdbarch.sh
  gdb: fix shellcheck warnings SC2034 (unused variable)  in gdbarch.sh
  gdb: fix shellcheck warnings SC2154 (referenced but not assigned) in
    gdbarch.sh
  gdb: silence shellcheck warning SC2162 (use read -r) in gdbarch.sh

 gdb/gdbarch.sh | 178 ++++++++++++++++++++++---------------------------
 1 file changed, 81 insertions(+), 97 deletions(-)

-- 
2.26.2

[PATCH 1/7] gdb: fix shellcheck warnings SC2059 (variables in printf format string) in gdbarch.sh

[Simon Marchi]

Fix all instances of this:

    In gdbarch.sh line 2182:
                printf "  gdb_assert (!(${invalid_p}));\n"
                       ^-- SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo".

... by doing exactly as the message suggests.

The rationale explained here [1] makes sense, if there happens to be a
format specifier in text substituted for the variable, the printf won't
do what we expect.

[1] https://github.com/koalaman/shellcheck/wiki/SC2059

gdb/ChangeLog:

	* gdbarch.sh: Use %s with printf, instead of variables in the
	format string.
---
 gdb/gdbarch.sh | 122 ++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 5a39dec83da2..2780a819f915 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1409,8 +1409,8 @@ do
     if class_is_info_p
     then
 	printf "\n"
-	printf "extern ${returntype} gdbarch_${function} (struct gdbarch *gdbarch);\n"
-	printf "/* set_gdbarch_${function}() - not applicable - pre-initialized.  */\n"
+	printf "extern %s gdbarch_%s (struct gdbarch *gdbarch);\n" "$returntype" "$function"
+	printf "/* set_gdbarch_%s() - not applicable - pre-initialized.  */\n" "$function"
     fi
 done
 
@@ -1431,33 +1431,33 @@ do
     if class_is_predicate_p
     then
 	printf "\n"
-	printf "extern int gdbarch_${function}_p (struct gdbarch *gdbarch);\n"
+	printf "extern int gdbarch_%s_p (struct gdbarch *gdbarch);\n" "$function"
     fi
     if class_is_variable_p
     then
 	printf "\n"
-	printf "extern ${returntype} gdbarch_${function} (struct gdbarch *gdbarch);\n"
-	printf "extern void set_gdbarch_${function} (struct gdbarch *gdbarch, ${returntype} ${function});\n"
+	printf "extern %s gdbarch_%s (struct gdbarch *gdbarch);\n" "$returntype" "$function"
+	printf "extern void set_gdbarch_%s (struct gdbarch *gdbarch, %s %s);\n" "$function" "$returntype" "$function"
     fi
     if class_is_function_p
     then
 	printf "\n"
 	if [ "x${formal}" = "xvoid" ] && class_is_multiarch_p
 	then
-	    printf "typedef ${returntype} (gdbarch_${function}_ftype) (struct gdbarch *gdbarch);\n"
+	    printf "typedef %s (gdbarch_%s_ftype) (struct gdbarch *gdbarch);\n" "$returntype" "$function"
 	elif class_is_multiarch_p
 	then
-	    printf "typedef ${returntype} (gdbarch_${function}_ftype) (struct gdbarch *gdbarch, ${formal});\n"
+	    printf "typedef %s (gdbarch_%s_ftype) (struct gdbarch *gdbarch, %s);\n" "$returntype" "$function" "$formal"
 	else
-	    printf "typedef ${returntype} (gdbarch_${function}_ftype) (${formal});\n"
+	    printf "typedef %s (gdbarch_%s_ftype) (%s);\n" "$returntype" "$function" "$formal"
 	fi
 	if [ "x${formal}" = "xvoid" ]
 	then
-	  printf "extern ${returntype} gdbarch_${function} (struct gdbarch *gdbarch);\n"
+	  printf "extern %s gdbarch_%s (struct gdbarch *gdbarch);\n" "$returntype" "$function"
 	else
-	  printf "extern ${returntype} gdbarch_${function} (struct gdbarch *gdbarch, ${formal});\n"
+	  printf "extern %s gdbarch_%s (struct gdbarch *gdbarch, %s);\n" "$returntype" "$function" "$formal"
 	fi
-	printf "extern void set_gdbarch_${function} (struct gdbarch *gdbarch, gdbarch_${function}_ftype *${function});\n"
+	printf "extern void set_gdbarch_%s (struct gdbarch *gdbarch, gdbarch_%s_ftype *%s);\n" "$function" "$function" "$function"
     fi
 done
 
@@ -1832,7 +1832,7 @@ function_list | while do_read
 do
     if class_is_info_p
     then
-	printf "  ${returntype} ${function};\n"
+	printf "  %s %s;\n" "$returntype" "$function"
     fi
 done
 printf "\n"
@@ -1873,10 +1873,10 @@ function_list | while do_read
 do
     if class_is_variable_p
     then
-	printf "  ${returntype} ${function};\n"
+	printf "  %s %s;\n" "$returntype" "$function"
     elif class_is_function_p
     then
-	printf "  gdbarch_${function}_ftype *${function};\n"
+	printf "  gdbarch_%s_ftype *%s;\n" "$function" "$function"
     fi
 done
 printf "};\n"
@@ -1912,7 +1912,7 @@ function_list | while do_read
 do
     if class_is_info_p
     then
-	printf "  gdbarch->${function} = info->${function};\n"
+	printf "  gdbarch->%s = info->%s;\n" "$function" "$function"
     fi
 done
 printf "\n"
@@ -1923,7 +1923,7 @@ do
     then
 	if [ -n "${predefault}" -a "x${predefault}" != "x0" ]
 	then
-	  printf "  gdbarch->${function} = ${predefault};\n"
+	  printf "  gdbarch->%s = %s;\n" "$function" "$predefault"
 	fi
     fi
 done
@@ -1996,31 +1996,31 @@ do
     then
 	if [ "x${invalid_p}" = "x0" ]
 	then
-	    printf "  /* Skip verify of ${function}, invalid_p == 0 */\n"
+	    printf "  /* Skip verify of %s, invalid_p == 0 */\n" "$function"
 	elif class_is_predicate_p
 	then
-	    printf "  /* Skip verify of ${function}, has predicate.  */\n"
+	    printf "  /* Skip verify of %s, has predicate.  */\n" "$function"
 	# FIXME: See do_read for potential simplification
  	elif [ -n "${invalid_p}" -a -n "${postdefault}" ]
 	then
-	    printf "  if (${invalid_p})\n"
-	    printf "    gdbarch->${function} = ${postdefault};\n"
+	    printf "  if (%s)\n" "$invalid_p"
+	    printf "    gdbarch->%s = %s;\n" "$function" "$postdefault"
 	elif [ -n "${predefault}" -a -n "${postdefault}" ]
 	then
-	    printf "  if (gdbarch->${function} == ${predefault})\n"
-	    printf "    gdbarch->${function} = ${postdefault};\n"
+	    printf "  if (gdbarch->%s == %s)\n" "$function" "$predefault"
+	    printf "    gdbarch->%s = %s;\n" "$function" "$postdefault"
 	elif [ -n "${postdefault}" ]
 	then
-	    printf "  if (gdbarch->${function} == 0)\n"
-	    printf "    gdbarch->${function} = ${postdefault};\n"
+	    printf "  if (gdbarch->%s == 0)\n" "$function"
+	    printf "    gdbarch->%s = %s;\n" "$function" "$postdefault"
 	elif [ -n "${invalid_p}" ]
 	then
-	    printf "  if (${invalid_p})\n"
-	    printf "    log.puts (\"\\\\n\\\\t${function}\");\n"
+	    printf "  if (%s)\n" "$invalid_p"
+	    printf "    log.puts (\"\\\\n\\\\t%s\");\n" "$function"
 	elif [ -n "${predefault}" ]
 	then
-	    printf "  if (gdbarch->${function} == ${predefault})\n"
-	    printf "    log.puts (\"\\\\n\\\\t${function}\");\n"
+	    printf "  if (gdbarch->%s == %s)\n" "$function" "$predefault"
+	    printf "    log.puts (\"\\\\n\\\\t%s\");\n" "$function"
 	fi
     fi
 done
@@ -2056,15 +2056,15 @@ do
     if class_is_predicate_p
     then
 	printf "  fprintf_unfiltered (file,\n"
-	printf "                      \"gdbarch_dump: gdbarch_${function}_p() = %%d\\\\n\",\n"
-	printf "                      gdbarch_${function}_p (gdbarch));\n"
+	printf "                      \"gdbarch_dump: gdbarch_%s_p() = %%d\\\\n\",\n" "$function"
+	printf "                      gdbarch_%s_p (gdbarch));\n" "$function"
     fi
     # Print the corresponding value.
     if class_is_function_p
     then
 	printf "  fprintf_unfiltered (file,\n"
-	printf "                      \"gdbarch_dump: ${function} = <%%s>\\\\n\",\n"
-	printf "                      host_address_to_string (gdbarch->${function}));\n"
+	printf "                      \"gdbarch_dump: %s = <%%s>\\\\n\",\n" "$function"
+	printf "                      host_address_to_string (gdbarch->%s));\n" "$function"
     else
 	# It is a variable
 	case "${print}:${returntype}" in
@@ -2081,8 +2081,8 @@ do
 		;;
         esac
 	printf "  fprintf_unfiltered (file,\n"
-	printf "                      \"gdbarch_dump: ${function} = %s\\\\n\",\n" "${fmt}"
-	printf "                      ${print});\n"
+	printf "                      \"gdbarch_dump: %s = %s\\\\n\",\n" "$function" "$fmt"
+	printf "                      %s);\n" "$print"
     fi
 done
 cat <<EOF
@@ -2110,32 +2110,32 @@ do
     then
 	printf "\n"
 	printf "int\n"
-	printf "gdbarch_${function}_p (struct gdbarch *gdbarch)\n"
+	printf "gdbarch_%s_p (struct gdbarch *gdbarch)\n" "$function"
 	printf "{\n"
         printf "  gdb_assert (gdbarch != NULL);\n"
-	printf "  return ${predicate};\n"
+	printf "  return %s;\n" "$predicate"
 	printf "}\n"
     fi
     if class_is_function_p
     then
 	printf "\n"
-	printf "${returntype}\n"
+	printf "%s\n" "$returntype"
 	if [ "x${formal}" = "xvoid" ]
 	then
-	  printf "gdbarch_${function} (struct gdbarch *gdbarch)\n"
+	  printf "gdbarch_%s (struct gdbarch *gdbarch)\n" "$function"
 	else
-	  printf "gdbarch_${function} (struct gdbarch *gdbarch, ${formal})\n"
+	  printf "gdbarch_%s (struct gdbarch *gdbarch, %s)\n" "$function" "$formal"
 	fi
 	printf "{\n"
         printf "  gdb_assert (gdbarch != NULL);\n"
-	printf "  gdb_assert (gdbarch->${function} != NULL);\n"
+	printf "  gdb_assert (gdbarch->%s != NULL);\n" "$function"
 	if class_is_predicate_p && test -n "${predefault}"
 	then
 	    # Allow a call to a function with a predicate.
-	    printf "  /* Do not check predicate: ${predicate}, allow call.  */\n"
+	    printf "  /* Do not check predicate: %s, allow call.  */\n" "$predicate"
 	fi
 	printf "  if (gdbarch_debug >= 2)\n"
-	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_${function} called\\\\n\");\n"
+	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_%s called\\\\n\");\n" "$function"
 	if [ "x${actual}" = "x-" -o "x${actual}" = "x" ]
 	then
 	    if class_is_multiarch_p
@@ -2154,58 +2154,58 @@ do
         fi
        	if [ "x${returntype}" = "xvoid" ]
 	then
-	  printf "  gdbarch->${function} (${params});\n"
+	  printf "  gdbarch->%s (%s);\n" "$function" "$params"
 	else
-	  printf "  return gdbarch->${function} (${params});\n"
+	  printf "  return gdbarch->%s (%s);\n" "$function" "$params"
 	fi
 	printf "}\n"
 	printf "\n"
 	printf "void\n"
-	printf "set_gdbarch_${function} (struct gdbarch *gdbarch,\n"
-        printf "            `echo ${function} | sed -e 's/./ /g'`  gdbarch_${function}_ftype ${function})\n"
+	printf "set_gdbarch_%s (struct gdbarch *gdbarch,\n" "$function"
+        printf "            `echo ${function} | sed -e 's/./ /g'`  gdbarch_%s_ftype %s)\n" "$function" "$function"
 	printf "{\n"
-	printf "  gdbarch->${function} = ${function};\n"
+	printf "  gdbarch->%s = %s;\n" "$function" "$function"
 	printf "}\n"
     elif class_is_variable_p
     then
 	printf "\n"
-	printf "${returntype}\n"
-	printf "gdbarch_${function} (struct gdbarch *gdbarch)\n"
+	printf "%s\n" "$returntype"
+	printf "gdbarch_%s (struct gdbarch *gdbarch)\n" "$function"
 	printf "{\n"
         printf "  gdb_assert (gdbarch != NULL);\n"
 	if [ "x${invalid_p}" = "x0" ]
 	then
-	    printf "  /* Skip verify of ${function}, invalid_p == 0 */\n"
+	    printf "  /* Skip verify of %s, invalid_p == 0 */\n" "$function"
 	elif [ -n "${invalid_p}" ]
 	then
 	    printf "  /* Check variable is valid.  */\n"
-	    printf "  gdb_assert (!(${invalid_p}));\n"
+	    printf "  gdb_assert (!(%s));\n" "$invalid_p"
 	elif [ -n "${predefault}" ]
 	then
 	    printf "  /* Check variable changed from pre-default.  */\n"
-	    printf "  gdb_assert (gdbarch->${function} != ${predefault});\n"
+	    printf "  gdb_assert (gdbarch->%s != %s);\n" "$function" "$predefault"
 	fi
 	printf "  if (gdbarch_debug >= 2)\n"
-	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_${function} called\\\\n\");\n"
-	printf "  return gdbarch->${function};\n"
+	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_%s called\\\\n\");\n" "$function"
+	printf "  return gdbarch->%s;\n" "$function"
 	printf "}\n"
 	printf "\n"
 	printf "void\n"
-	printf "set_gdbarch_${function} (struct gdbarch *gdbarch,\n"
-        printf "            `echo ${function} | sed -e 's/./ /g'`  ${returntype} ${function})\n"
+	printf "set_gdbarch_%s (struct gdbarch *gdbarch,\n" "$function"
+        printf "            `echo ${function} | sed -e 's/./ /g'`  %s %s)\n" "$returntype" "$function"
 	printf "{\n"
-	printf "  gdbarch->${function} = ${function};\n"
+	printf "  gdbarch->%s = %s;\n" "$function" "$function"
 	printf "}\n"
     elif class_is_info_p
     then
 	printf "\n"
-	printf "${returntype}\n"
-	printf "gdbarch_${function} (struct gdbarch *gdbarch)\n"
+	printf "%s\n" "$returntype"
+	printf "gdbarch_%s (struct gdbarch *gdbarch)\n" "$function"
 	printf "{\n"
         printf "  gdb_assert (gdbarch != NULL);\n"
 	printf "  if (gdbarch_debug >= 2)\n"
-	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_${function} called\\\\n\");\n"
-	printf "  return gdbarch->${function};\n"
+	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_%s called\\\\n\");\n" "$function"
+	printf "  return gdbarch->%s;\n" "$function"
 	printf "}\n"
     fi
 done
-- 
2.26.2

[PATCH 2/7] gdb: fix shellcheck warnings SC2086 (missing double quotes) in gdbarch.sh

[Simon Marchi]

Fix all instances of:

    In gdbarch.sh line 31:
        if test ! -r ${file}
                     ^-----^ SC2086: Double quote to prevent globbing and word splitting.

    Did you mean:
        if test ! -r "${file}"

Note that some instances of these are in text that is eval'ed.  I'm
pretty sure that things could go wrong during the eval too, but that's
not something shellcheck can check.

gdb/ChangeLog:

	* gdbarch.sh: Use double quotes around variables.
---
 gdb/gdbarch.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 2780a819f915..a934a7aa6bc1 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -28,10 +28,10 @@ LC_ALL=C ; export LC_ALL
 compare_new ()
 {
     file=$1
-    if test ! -r ${file}
+    if test ! -r "${file}"
     then
 	echo "${file} missing? cp new-${file} ${file}" 1>&2
-    elif diff -u ${file} new-${file}
+    elif diff -u "${file}" "new-${file}"
     then
 	echo "${file} unchanged" 1>&2
     else
@@ -70,7 +70,7 @@ ${line}"
 	    line="`echo "${line}" | sed -e 's/;;/; ;/g' -e 's/;;/; ;/g'`"
 
 	    OFS="${IFS}" ; IFS="[;]"
-	    eval read ${read} <<EOF
+	    eval read "${read}" <<EOF
 ${line}
 EOF
 	    IFS="${OFS}"
@@ -86,9 +86,9 @@ EOF
 	    # that ended up with just that space character.
 	    for r in ${read}
 	    do
-		if eval test \"\${${r}}\" = \"\ \"
+		if eval test "\"\${${r}}\" = ' '"
 		then
-		    eval ${r}=""
+		    eval "${r}="
 		fi
 	    done
 
@@ -1227,7 +1227,7 @@ ${class} ${returntype} ${function} ($formal)
 EOF
     for r in ${read}
     do
-	eval echo \"\ \ \ \ ${r}=\${${r}}\"
+	eval echo "\"    ${r}=\${${r}}\""
     done
     if class_is_predicate_p && fallback_default_p
     then
@@ -2162,7 +2162,7 @@ do
 	printf "\n"
 	printf "void\n"
 	printf "set_gdbarch_%s (struct gdbarch *gdbarch,\n" "$function"
-        printf "            `echo ${function} | sed -e 's/./ /g'`  gdbarch_%s_ftype %s)\n" "$function" "$function"
+        printf "            `echo "$function" | sed -e 's/./ /g'`  gdbarch_%s_ftype %s)\n" "$function" "$function"
 	printf "{\n"
 	printf "  gdbarch->%s = %s;\n" "$function" "$function"
 	printf "}\n"
@@ -2192,7 +2192,7 @@ do
 	printf "\n"
 	printf "void\n"
 	printf "set_gdbarch_%s (struct gdbarch *gdbarch,\n" "$function"
-        printf "            `echo ${function} | sed -e 's/./ /g'`  %s %s)\n" "$returntype" "$function"
+        printf "            `echo "$function" | sed -e 's/./ /g'`  %s %s)\n" "$returntype" "$function"
 	printf "{\n"
 	printf "  gdbarch->%s = %s;\n" "$function" "$function"
 	printf "}\n"
-- 
2.26.2

[PATCH 3/7] gdb: fix shellcheck warnings SC2006 (use $() instead of ``) in gdbarch.sh

[Simon Marchi]

Fix all instances of:

    In gdbarch.sh line 2195:
            printf "            `echo "$function" | sed -e 's/./ /g'`  %s %s)\n" "$returntype" "$function"
                                ^-- SC2006: Use $(...) notation instead of legacy backticked `...`.

    Did you mean:
            printf "            $(echo "$function" | sed -e 's/./ /g')  %s %s)\n" "$returntype" "$function"

See here [1] for the rationale.

[1] https://github.com/koalaman/shellcheck/wiki/SC2006

gdb/ChangeLog:

	* gdbarch.sh: Use $(...) instead of `...`.
---
 gdb/gdbarch.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index a934a7aa6bc1..0e89bd1900b3 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -67,7 +67,7 @@ ${line}"
 	    # The semantics of IFS varies between different SH's.  Some
 	    # treat ``;;' as three fields while some treat it as just two.
 	    # Work around this by eliminating ``;;'' ....
-	    line="`echo "${line}" | sed -e 's/;;/; ;/g' -e 's/;;/; ;/g'`"
+	    line="$(echo "${line}" | sed -e 's/;;/; ;/g' -e 's/;;/; ;/g')"
 
 	    OFS="${IFS}" ; IFS="[;]"
 	    eval read "${read}" <<EOF
@@ -2162,7 +2162,7 @@ do
 	printf "\n"
 	printf "void\n"
 	printf "set_gdbarch_%s (struct gdbarch *gdbarch,\n" "$function"
-        printf "            `echo "$function" | sed -e 's/./ /g'`  gdbarch_%s_ftype %s)\n" "$function" "$function"
+	printf "            %s  gdbarch_%s_ftype %s)\n" "$(echo "$function" | sed -e 's/./ /g')" "$function" "$function"
 	printf "{\n"
 	printf "  gdbarch->%s = %s;\n" "$function" "$function"
 	printf "}\n"
@@ -2192,7 +2192,7 @@ do
 	printf "\n"
 	printf "void\n"
 	printf "set_gdbarch_%s (struct gdbarch *gdbarch,\n" "$function"
-        printf "            `echo "$function" | sed -e 's/./ /g'`  %s %s)\n" "$returntype" "$function"
+	printf "            %s  %s %s)\n" "$(echo "$function" | sed -e 's/./ /g')" "$returntype" "$function"
 	printf "{\n"
 	printf "  gdbarch->%s = %s;\n" "$function" "$function"
 	printf "}\n"
-- 
2.26.2

[PATCH 4/7] gdb: fix shellcheck warnings SC2166 (&& and !! instead of -a and -o) in gdbarch.sh

[Simon Marchi]

Fix all warnings of this type:

    In gdbarch.sh line 1238:
        if [ "x${invalid_p}" = "x0" -a -n "${postdefault}" ]
                                    ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.

See the rationale here [1].

[1] https://github.com/koalaman/shellcheck/wiki/SC2166

gdb/ChangeLog:

	* gdbarch.sh: Use shell operators && and || instead of
	-a and -o.
---
 gdb/gdbarch.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 0e89bd1900b3..f1b9276775e3 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -156,8 +156,8 @@ EOF
 
 fallback_default_p ()
 {
-    [ -n "${postdefault}" -a "x${invalid_p}" != "x0" ] \
-	|| [ -n "${predefault}" -a "x${invalid_p}" = "x0" ]
+    ( [ -n "${postdefault}" ] && [ "x${invalid_p}" != "x0" ] ) \
+	|| ( [ -n "${predefault}" ] && [ "x${invalid_p}" = "x0" ] )
 }
 
 class_is_variable_p ()
@@ -1235,7 +1235,7 @@ EOF
 	kill $$
 	exit 1
     fi
-    if [ "x${invalid_p}" = "x0" -a -n "${postdefault}" ]
+    if [ "x${invalid_p}" = "x0" ] && [ -n "${postdefault}" ]
     then
 	echo "Error: postdefault is useless when invalid_p=0" 1>&2
 	kill $$
@@ -1921,7 +1921,7 @@ function_list | while do_read
 do
     if class_is_function_p || class_is_variable_p
     then
-	if [ -n "${predefault}" -a "x${predefault}" != "x0" ]
+	if [ -n "${predefault}" ] && [ "x${predefault}" != "x0" ]
 	then
 	  printf "  gdbarch->%s = %s;\n" "$function" "$predefault"
 	fi
@@ -2001,11 +2001,11 @@ do
 	then
 	    printf "  /* Skip verify of %s, has predicate.  */\n" "$function"
 	# FIXME: See do_read for potential simplification
- 	elif [ -n "${invalid_p}" -a -n "${postdefault}" ]
+	elif [ -n "${invalid_p}" ] && [ -n "${postdefault}" ]
 	then
 	    printf "  if (%s)\n" "$invalid_p"
 	    printf "    gdbarch->%s = %s;\n" "$function" "$postdefault"
-	elif [ -n "${predefault}" -a -n "${postdefault}" ]
+	elif [ -n "${predefault}" ] && [ -n "${postdefault}" ]
 	then
 	    printf "  if (gdbarch->%s == %s)\n" "$function" "$predefault"
 	    printf "    gdbarch->%s = %s;\n" "$function" "$postdefault"
@@ -2136,7 +2136,7 @@ do
 	fi
 	printf "  if (gdbarch_debug >= 2)\n"
 	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_%s called\\\\n\");\n" "$function"
-	if [ "x${actual}" = "x-" -o "x${actual}" = "x" ]
+	if [ "x${actual}" = "x-" ] || [ "x${actual}" = "x" ]
 	then
 	    if class_is_multiarch_p
 	    then
-- 
2.26.2

[PATCH 5/7] gdb: fix shellcheck warnings SC2034 (unused variable) in gdbarch.sh

[Simon Marchi]

shellcheck reports:

    In gdbarch.sh line 139:
                    fallbackdefault="0"
                    ^-------------^ SC2034: fallbackdefault appears unused. Verify use (or export if used externally).

Indeed, the `fallbackdefault` variable appears to be unused, remove the
code that sets it.

gdb/ChangeLog:

	* gdbarch.sh: Remove code that sets fallbackdefault.
---
 gdb/gdbarch.sh | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index f1b9276775e3..4e4cc827b895 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -122,23 +122,6 @@ EOF
 		esac
 	    esac
 
-	    # PREDEFAULT is a valid fallback definition of MEMBER when
-	    # multi-arch is not enabled.  This ensures that the
-	    # default value, when multi-arch is the same as the
-	    # default value when not multi-arch.  POSTDEFAULT is
-	    # always a valid definition of MEMBER as this again
-	    # ensures consistency.
-
-	    if [ -n "${postdefault}" ]
-	    then
-		fallbackdefault="${postdefault}"
-	    elif [ -n "${predefault}" ]
-	    then
-		fallbackdefault="${predefault}"
-	    else
-		fallbackdefault="0"
-	    fi
-
 	    #NOT YET: See gdbarch.log for basic verification of
 	    # database
 
@@ -156,8 +139,8 @@ EOF
 
 fallback_default_p ()
 {
-    ( [ -n "${postdefault}" ] && [ "x${invalid_p}" != "x0" ] ) \
-	|| ( [ -n "${predefault}" ] && [ "x${invalid_p}" = "x0" ] )
+    { [ -n "${postdefault}" ] && [ "x${invalid_p}" != "x0" ]; } \
+	|| { [ -n "${predefault}" ] && [ "x${invalid_p}" = "x0" ]; }
 }
 
 class_is_variable_p ()
-- 
2.26.2

[PATCH 6/7] gdb: fix shellcheck warnings SC2154 (referenced but not assigned) in gdbarch.sh

[Simon Marchi]

Fix all instances of this kind of warning:

    In gdbarch.sh line 96:
                    m ) staticdefault="${predefault}" ;;
                                       ^-----------^ SC2154: predefault is referenced but not assigned.

These warnings appear because we are doing something a bit funky when reading
the gdbarch fields.  These variables are not assigned explicitly, but
using some `eval` commands.

I don't think there is so much we can fix about those warnings.  To
silence them, I've changed `${foo}` to `${foo:-}`.  This tells the shell
to substitute with an empty string if `foo` is not defined.  This
retains the current behavior, but the warnings go away.

gdb/ChangeLog:

	* gdbarch.sh: Use ${foo:-} where shellcheck would report a
	"referenced but not assigned" warning.
---
 gdb/gdbarch.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 4e4cc827b895..24f8cdfe90b0 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -75,7 +75,7 @@ ${line}
 EOF
 	    IFS="${OFS}"
 
-	    if test -n "${garbage_at_eol}"
+	    if test -n "${garbage_at_eol:-}"
 	    then
 		echo "Garbage at end-of-line in ${line}" 1>&2
 		kill $$
@@ -93,19 +93,19 @@ EOF
 	    done
 
 	    case "${class}" in
-		m ) staticdefault="${predefault}" ;;
+		m ) staticdefault="${predefault:-}" ;;
 		M ) staticdefault="0" ;;
 		* ) test "${staticdefault}" || staticdefault=0 ;;
 	    esac
 
 	    case "${class}" in
 	    F | V | M )
-		case "${invalid_p}" in
+		case "${invalid_p:-}" in
 		"" )
 		    if test -n "${predefault}"
 		    then
 			#invalid_p="gdbarch->${function} == ${predefault}"
-			predicate="gdbarch->${function} != ${predefault}"
+			predicate="gdbarch->${function:-} != ${predefault}"
 		    elif class_is_variable_p
 		    then
 			predicate="gdbarch->${function} != 0"
@@ -139,7 +139,7 @@ EOF
 
 fallback_default_p ()
 {
-    { [ -n "${postdefault}" ] && [ "x${invalid_p}" != "x0" ]; } \
+    { [ -n "${postdefault:-}" ] && [ "x${invalid_p}" != "x0" ]; } \
 	|| { [ -n "${predefault}" ] && [ "x${invalid_p}" = "x0" ]; }
 }
 
@@ -1206,7 +1206,7 @@ exec > new-gdbarch.log
 function_list | while do_read
 do
     cat <<EOF
-${class} ${returntype} ${function} ($formal)
+${class} ${returntype:-} ${function} (${formal:-})
 EOF
     for r in ${read}
     do
@@ -2119,7 +2119,7 @@ do
 	fi
 	printf "  if (gdbarch_debug >= 2)\n"
 	printf "    fprintf_unfiltered (gdb_stdlog, \"gdbarch_%s called\\\\n\");\n" "$function"
-	if [ "x${actual}" = "x-" ] || [ "x${actual}" = "x" ]
+	if [ "x${actual:-}" = "x-" ] || [ "x${actual:-}" = "x" ]
 	then
 	    if class_is_multiarch_p
 	    then
-- 
2.26.2

[PATCH 7/7] gdb: silence shellcheck warning SC2162 (use read -r) in gdbarch.sh

[Simon Marchi]

shellcheck reports:

    In gdbarch.sh line 53:
        while IFS='' read line
                     ^--^ SC2162: read without -r will mangle backslashes.

See the rationale at [1].  In our case, we actually want the backslashes
to be interpreted and removed.  Silence the warning using a directive.

[1] https://github.com/koalaman/shellcheck/wiki/SC2162

gdb/ChangeLog:

	* gdbarch.sh (do_read): Add shellcheck disable directive for
	warning SC2162.
---
 gdb/gdbarch.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 24f8cdfe90b0..13775078c25a 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -50,6 +50,7 @@ do_read ()
     # On some SH's, 'read' trims leading and trailing whitespace by
     # default (e.g., bash), while on others (e.g., dash), it doesn't.
     # Set IFS to empty to disable the trimming everywhere.
+    # shellcheck disable=SC2162
     while IFS='' read line
     do
 	if test "${line}" = ""
-- 
2.26.2

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I ran shellcheck on gdbarch.sh and addressed all the warnings.  It
Simon> didn't catch anything serious, but I think it's good to have it clean
Simon> anyway, so we can catch potential problems in future changes we do to
Simon> this file.

These all seemed fine to me.

I'd like to see gdbarch.sh eventually go away entirely.
Most of it could be ordinary C++ code.  I don't have a concrete plan for
this though.  Mostly I've been reluctant to do it due to the amount of
reindentation that will probably be involved, though I guess maybe  I
could write an emacs lisp script to handle this.

Tom

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Simon Marchi]

On 2020-04-29 5:08 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> I ran shellcheck on gdbarch.sh and addressed all the warnings.  It
> Simon> didn't catch anything serious, but I think it's good to have it clean
> Simon> anyway, so we can catch potential problems in future changes we do to
> Simon> this file.
> 
> These all seemed fine to me.

Thanks, I'll push it.

> I'd like to see gdbarch.sh eventually go away entirely.
> Most of it could be ordinary C++ code.  I don't have a concrete plan for
> this though.  Mostly I've been reluctant to do it due to the amount of
> reindentation that will probably be involved, though I guess maybe  I
> could write an emacs lisp script to handle this.

I think that gdbarch.sh will go away eventually too, but probably not soon,
so I thought it would still be worth it to do this work.

Even if you don't plan to do it, if you have ideas of how we could do the
equivalent in pure C++, it would be nice if you could spell them out somewhere.
I think about this some times, but I don't have a clear picture of how the
result could look like.

Simon

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Tom Tromey]

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

Simon> Even if you don't plan to do it, if you have ideas of how we could do the
Simon> equivalent in pure C++, it would be nice if you could spell them out somewhere.
Simon> I think about this some times, but I don't have a clear picture of how the
Simon> result could look like.

I was thinking that most fields would have a type like:

template<typename T>
class gdbarch_wrapper
{
  T m_data;

  T operator() () const { return m_data; }
  void set (const T &v) { m_data = v; }
};

So, byte order would be

    gdbarch_wrapper<bfd_endian> byte_order;

Calls like set_gdbarch_byte_order(x,y) would be x->byte_order.set(y);
gdbarch_byte_order(x) would be x->byte_order();

Maybe this doesn't provide much value over just plain fields.


Types with a predicate would have an additional "is_set" method and
maybe wrap an option<> (or similar -- we could probably specialize this
for pointers to just check for NULL).

Function fields would have an operator() that would forward the
arguments.


I'm not sure how best to handle method fields, where the gdbarch is
passed through.  This part makes me suspect that this isn't a good plan.

Maybe these could be ordinary methods that forward through the field,
with a new type here so that the field's operator() isn't accessible
outside struct gdbarch.


I suppose gdbarch_wrapper<T>::operator() could handle the gdbarch
debugging stuff.  I personally would be inclined to remove it -- I have
never once used it -- but maybe that's just me.  Maybe it could be
retained only for function calls.


I wrote a bit of elisp last night to do the basic transform (function
calls to method calls).  That part doesn't seem so hard.  Harder is
making it a reviewable series, instead of one giant bomb.

One plan would be to move struct gdbarch to gdbarch.h (removing the
read-only-ness of these files); then convert one field at a time,
removing the related old-style functions; then any final cleanups.

WDYT?

Tom

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Simon Marchi]

On 2020-04-30 10:25 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Even if you don't plan to do it, if you have ideas of how we could do the
> Simon> equivalent in pure C++, it would be nice if you could spell them out somewhere.
> Simon> I think about this some times, but I don't have a clear picture of how the
> Simon> result could look like.
> 
> I was thinking that most fields would have a type like:
> 
> template<typename T>
> class gdbarch_wrapper
> {
>   T m_data;
> 
>   T operator() () const { return m_data; }
>   void set (const T &v) { m_data = v; }
> };
> 
> So, byte order would be
> 
>     gdbarch_wrapper<bfd_endian> byte_order;
> 
> Calls like set_gdbarch_byte_order(x,y) would be x->byte_order.set(y);
> gdbarch_byte_order(x) would be x->byte_order();
> 
> Maybe this doesn't provide much value over just plain fields.
> 
> 
> Types with a predicate would have an additional "is_set" method and
> maybe wrap an option<> (or similar -- we could probably specialize this
> for pointers to just check for NULL).
> 
> Function fields would have an operator() that would forward the
> arguments.
> 
> 
> I'm not sure how best to handle method fields, where the gdbarch is
> passed through.  This part makes me suspect that this isn't a good plan.
> 
> Maybe these could be ordinary methods that forward through the field,
> with a new type here so that the field's operator() isn't accessible
> outside struct gdbarch.
> 
> 
> I suppose gdbarch_wrapper<T>::operator() could handle the gdbarch
> debugging stuff.  I personally would be inclined to remove it -- I have
> never once used it -- but maybe that's just me.  Maybe it could be
> retained only for function calls.
> 
> 
> I wrote a bit of elisp last night to do the basic transform (function
> calls to method calls).  That part doesn't seem so hard.  Harder is
> making it a reviewable series, instead of one giant bomb.
> 
> One plan would be to move struct gdbarch to gdbarch.h (removing the
> read-only-ness of these files); then convert one field at a time,
> removing the related old-style functions; then any final cleanups.
> 
> WDYT?
> 
> Tom
> 

Ok, so the gdbarch object would stay somewhat similar to what it is now,
it would contain function pointers that can be set at runtime.

Somehow, I was trying to think of how to do it by defining gdbarch as
a base class with gdbarch methods as virtual methods of this class.
But that does not work well with how gdbarch objects are constructed
today.  We would have to define one concrete subclass for each possible
resulting gdbarch.  I don't think that's possible, as there are just too
many combinations.

Your plan is more realistic.

Simon

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Tom Tromey]

Tom> I suppose gdbarch_wrapper<T>::operator() could handle the gdbarch
Tom> debugging stuff.

Actually, it can't really, unless we want to capture the gdbarch in many
of these fields.

Tom> I personally would be inclined to remove it

Still true for me, but not something I'd pursue unilaterally.

Tom

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Pedro Alves]

On 4/29/20 10:08 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> I ran shellcheck on gdbarch.sh and addressed all the warnings.  It
> Simon> didn't catch anything serious, but I think it's good to have it clean
> Simon> anyway, so we can catch potential problems in future changes we do to
> Simon> this file.
> 
> These all seemed fine to me.
> 
> I'd like to see gdbarch.sh eventually go away entirely.
> Most of it could be ordinary C++ code.  I don't have a concrete plan for
> this though.  Mostly I've been reluctant to do it due to the amount of
> reindentation that will probably be involved, though I guess maybe  I
> could write an emacs lisp script to handle this.

My main gripe with gdbarch.sh is that the function/variable/method definitions
and the generator code is all in the same file.

If those were split to separate files, like, the definitions inside function_list()
were moved to a separate gdbarch.def file, which would be read by gdbarch.sh,
that'd already be a large win, IMHO.

Also, I would like it to be able to generate the gdbarch.h/c files in place,
instead of generating new "new-gdbarch.h/c" files.

Thanks,
Pedro Alves

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Simon Marchi]

On 2020-05-10 2:57 p.m., Pedro Alves wrote:
> On 4/29/20 10:08 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>> Simon> I ran shellcheck on gdbarch.sh and addressed all the warnings.  It
>> Simon> didn't catch anything serious, but I think it's good to have it clean
>> Simon> anyway, so we can catch potential problems in future changes we do to
>> Simon> this file.
>>
>> These all seemed fine to me.
>>
>> I'd like to see gdbarch.sh eventually go away entirely.
>> Most of it could be ordinary C++ code.  I don't have a concrete plan for
>> this though.  Mostly I've been reluctant to do it due to the amount of
>> reindentation that will probably be involved, though I guess maybe  I
>> could write an emacs lisp script to handle this.
> 
> My main gripe with gdbarch.sh is that the function/variable/method definitions
> and the generator code is all in the same file.
> 
> If those were split to separate files, like, the definitions inside function_list()
> were moved to a separate gdbarch.def file, which would be read by gdbarch.sh,
> that'd already be a large win, IMHO.

If I was to do it from scratch, I'd do the definitions in a yaml file with a python
script as the generator.  If you would be fine with that, I'd be happy to try it as
a weekend project.  I just never proposed it because I thought people were happy with
what we currently have and wouldn't welcome changing something for the sake of
changing it :).

> Also, I would like it to be able to generate the gdbarch.h/c files in place,
> instead of generating new "new-gdbarch.h/c" files.

Same here.  I never thought about that before, but it probably comes from the
pre-git era, where it would have been more difficult to compare the before and after
if the file was written in place?

Simon

Re: [PATCH 0/7] Make gdbarch.sh shellcheck-clean

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

Pedro> My main gripe with gdbarch.sh is that the function/variable/method definitions
Pedro> and the generator code is all in the same file.

Pedro> If those were split to separate files, like, the definitions inside function_list()
Pedro> were moved to a separate gdbarch.def file, which would be read by gdbarch.sh,
Pedro> that'd already be a large win, IMHO.

Pedro> Also, I would like it to be able to generate the gdbarch.h/c files in place,
Pedro> instead of generating new "new-gdbarch.h/c" files.

Another option would be to go with a .def-style file, but using #include
and #define to turn it into C++ code, like we do with other files.

There's basically two things I don't like about the current setup.

One problem is that the current code is hard to read.  Any of the ideas
we've discussed would fix this.

The other problem is not just that the output files are new-gdbarch.[ch]
-- that can be fixed with a simple script tweak -- but that the script
has to be run manually to update & commit the output.  This bites me
occasionally; it would be preferable by far if it were simply integrated
into the build in a normal way.

This latter thing is a problem with make-target-delegates as well,
though I touch that one a lot less often, so it doesn't irritate as
much.

Tom