[PATCH] D56180: Replace gen_dynamic_list.py with a portable shell script

Michał Górny via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 31 21:03:20 PST 2018


mgorny added a comment.

Also, note that technically all commands in shell may fail, and especially commands operating on external files, so you may consider adding some more error handling. You may get very weird results e.g. if you ran out of space in the middle of the script.



================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:19
+usage() {
+	echo "usage: gen_dynamic_list.py [-h] [--version-list] [--extra EXTRA]"
+	echo "                           libraries [libraries ...]"
----------------
You still have `.py` here.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:33
+	$my_nm $1 > $tmpfile1
+	if [ $? -ne 0 ]; then
+		printf 'Error calling: %s %s... bailing out\n' "$my_nm" "$1"
----------------
You can fold the command inside, i.e. `if ! $my_nm ...; then`


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:34
+	if [ $? -ne 0 ]; then
+		printf 'Error calling: %s %s... bailing out\n' "$my_nm" "$1"
+		exit 1
----------------
Why are you using `printf` when the same result could be achieved by simpler `echo`?


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:42
+
+version_list=
+extra=
----------------
By convention, global variables in shell script should be uppercase.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:45
+
+tmpfile1=`mktemp /tmp/${0##*/}.XXXXXX` || exit 1
+tmpfile2=`mktemp /tmp/${0##*/}.XXXXXX` || exit 1
----------------
You probably want to respect `$TMPDIR` here. Also, `$(...)` is strongly preferred over backticks which are unreadable and non-nesting friendly. Quote the filenames, in case TMPDIR contained spaces.

And, well, speaking of portability `mktemp(1)` is not POSIX.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:48
+
+trap "rm -f $tmpfile1 $tmpfile2" EXIT
+
----------------
Quote the file paths inside too.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:50
+
+my_nm=${NM:-nm}
+my_awk=${AWK:-awk}
----------------
Why not

    : "${NM:=nm}"

?


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:61
+		--extra)
+			if [ "x$2" = "x" ]; then
+				usage
----------------
Don't use the `x` hack, it is really, really obsolete and even autotools discourage that by now.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:63
+				usage
+				printf 'Error missing argument for the option\n'
+			else
----------------
echo

Also, don't you want to `exit 1` or like on error?


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:177
+	}
+	if (substr(\$0, 1, 14) == \"__interceptor_\") {
+		result[len++] = \$0
----------------
I think the standard awk way would be to use `~` regex match here.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:199
+}
+" $tmpfile2 $tmpfile2 > $tmpfile1
+
----------------
Quote file paths here and below.

Why are you passing `$tmpfile2` twice?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56180/new/

https://reviews.llvm.org/D56180





More information about the llvm-commits mailing list