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

Kamil Rytarowski via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 31 21:32:24 PST 2018


krytarowski marked 8 inline comments as done.
krytarowski 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.

Is it fine to go here for `set -e`?



================
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"
----------------
mgorny wrote:
> You can fold the command inside, i.e. `if ! $my_nm ...; then`
Right, but it's easier for debugging this code with unfolded commands.


================
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
----------------
mgorny wrote:
> Why are you using `printf` when the same result could be achieved by simpler `echo`?
No reason, I can switch to echo.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:42
+
+version_list=
+extra=
----------------
mgorny wrote:
> By convention, global variables in shell script should be uppercase.
Style in LLVM? It depends whom you ask. Uppercase symbols are for global variables pulled from environment. If this is the style of LLVM, I will align to it.


================
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
----------------
mgorny wrote:
> 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.
Right with `$TMPDIR`.

Is there a better replacement for `mktemp(1)`? It's available in all modern BSDs, Darwin, GNU and commercial Unices (Solaris, HP/UX). The API behind the command is POSIX though (`mktemp(3)`).

And if I would need to invent mktemp(1) with a handwritten random number generator, it's tricky to get such number as well.

backticks vs `$()` is a matter of taste here, if LLVM recommends one or the other, I will switch it (or keep it).


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:61
+		--extra)
+			if [ "x$2" = "x" ]; then
+				usage
----------------
mgorny wrote:
> Don't use the `x` hack, it is really, really obsolete and even autotools discourage that by now.
It's not a hack, but portability idiom. There might be issues with evaluating empty strings `""` or using `-z`.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:63
+				usage
+				printf 'Error missing argument for the option\n'
+			else
----------------
mgorny wrote:
> echo
> 
> Also, don't you want to `exit 1` or like on error?
Right, missing `exit`.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:177
+	}
+	if (substr(\$0, 1, 14) == \"__interceptor_\") {
+		result[len++] = \$0
----------------
mgorny wrote:
> I think the standard awk way would be to use `~` regex match here.
For some reason `^_interceptor_` didn't work as expected and there is need to skip namespaced (and mangled) symbols. We always know the length/position here so it's a straightforward solution.


================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.sh:199
+}
+" $tmpfile2 $tmpfile2 > $tmpfile1
+
----------------
mgorny wrote:
> Quote file paths here and below.
> 
> Why are you passing `$tmpfile2` twice?
There are two passes `NR==FNR {` to catch the first one and the other branch for the 2nd one.

Do we support building in paths with spaces in names of directories? It doesn't cost much to add quotes so I will add them.


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