diff mbox series

'gcc/config/nvptx/gen-multilib-matches.sh': Support '--selftest' (was: 'gcc/config/nvptx/t-nvptx': Don't use the 'shell' function of 'make' (was: nvptx: Allow '--with-arch' to override the default '-misa' (was: nvptx multilib setup)))

Message ID 87jzcd17iz.fsf@euler.schwinge.ddns.net
State New
Headers show
Series 'gcc/config/nvptx/gen-multilib-matches.sh': Support '--selftest' (was: 'gcc/config/nvptx/t-nvptx': Don't use the 'shell' function of 'make' (was: nvptx: Allow '--with-arch' to override the default '-misa' (was: nvptx multilib setup))) | expand

Commit Message

Thomas Schwinge Dec. 6, 2024, 9:32 a.m. UTC 
Hi!

On 2024-12-06T10:01:22+0100, I wrote:
> I recently learned that the exit status of the command invoked in a
> 'Makefile' via '$(shell [...])' effectively gets discarded (unless
> explicitly checking the GNU Make 4.2+ '.SHELLSTATUS' variable or jumping
> through other hoops).  I was under the assumption that an error in a
> 'shell' function would cause 'make' to error out, similarly to how it
> does in 'Makefile' rules...
>
> I learned this The Hard Way here:
>
> On 2022-06-15T23:18:10+0200, I wrote:
>> --- a/gcc/config/nvptx/t-nvptx
>> +++ b/gcc/config/nvptx/t-nvptx
>
>> +multilib_matches := $(shell $(srcdir)/config/nvptx/gen-multilib-matches.sh $(srcdir)/config/nvptx $(multilib_options_isa_default) "$(multilib_options_isa_list)")
>
> When recently working on changing nvptx multilib things, and for that
> enhancing nvptx' 'gen-multilib-matches.sh', I made an error in there, and
> then got confusing behavior in that I could still successfully 'make'
> GCC, and my changes "mostly appeared to work as expected", but not quite.
> This was due to garbage in 'MULTILIB_MATCHES', caused by a shell syntax
> error in 'gen-multilib-matches.sh' -- which '$(shell [...])' swept under
> the table.
>
> Pushed to trunk branch commit 490443357668a87e3c322f218873a7649a2552df
> "'gcc/config/nvptx/t-nvptx': Don't use the 'shell' function of 'make'",
> see attached.

To further improve reliability of nvptx 'gen-multilib-matches.sh', in
<https://inbox.sourceware.org/875xornmp9.fsf@euler.schwinge.ddns.net>
"Re: [PATCH] v2: Run selftests for C++ as well as C", I recently
mentioned the idea of adding selftesting to this script and attaching a
new 's-selftest-nvptx_gen-multilib-matches' rule to the existing GCC
selftest framework rules.  Instead, in a simpler way, let's just invoke
the 'gen-multilib-matches.sh --selftest' before actual use here:

> --- a/gcc/config/nvptx/t-nvptx
> +++ b/gcc/config/nvptx/t-nvptx
> @@ -43,12 +43,24 @@ MULTILIB_OPTIONS += mgomp
>  multilib_options_isa_list := $(TM_MULTILIB_CONFIG)
>  multilib_options_isa_default := $(word 1,$(multilib_options_isa_list))
>  multilib_options_misa_list := $(addprefix misa=,$(multilib_options_isa_list))
> +
> +t-nvptx-gen-multilib-matches: $(srcdir)/config/nvptx/gen-multilib-matches.sh \
> +  $(srcdir)/config/nvptx/t-nvptx \
> +  Makefile \
> +  $(srcdir)/config/nvptx/nvptx-sm.def
> +	$(SHELL) $< \
> +	  $(dir $<) \
> +	  $(multilib_options_isa_default) \
> +	  '$(multilib_options_isa_list)' \
> +	  > $@
> +
> +include t-nvptx-gen-multilib-matches

Pushed to trunk branch commit ccd6ec23177f7a4ed69fabad8e79d5d4da419fb2
"'gcc/config/nvptx/gen-multilib-matches.sh': Support '--selftest'", see
attached.


Grüße
 Thomas

Comments

Sam James Dec. 6, 2024, 9:34 a.m. UTC | #1 
Hi!

The script has #!/bin/sh shebang (and hence must have POSIX shell
compatibility), but the patch introduces uses of the 'local' keyword
which isn't in POSIX.

While many shells do have the 'local' keyword, its behaviour isn't
portable across those either, which is why it's likely it'll never
be added to POSIX :(

thanks,
sam
Sam James Dec. 6, 2024, 9:52 a.m. UTC | #2 
Sam James <sam@gentoo.org> writes:

> Hi!
>
> The script has #!/bin/sh shebang (and hence must have POSIX shell
> compatibility), but the patch introduces uses of the 'local' keyword
> which isn't in POSIX.
>
> While many shells do have the 'local' keyword, its behaviour isn't
> portable across those either, which is why it's likely it'll never
> be added to POSIX :(

BTW, shellcheck catches this, but unfortunately, checkbashisms does not.

>
> thanks,
> sam
Thomas Schwinge Dec. 6, 2024, 10:02 a.m. UTC | #3 
Hi Sam!

On 2024-12-06T09:34:32+0000, Sam James <sam@gentoo.org> wrote:
> The script has #!/bin/sh shebang (and hence must have POSIX shell
> compatibility), but the patch introduces uses of the 'local' keyword
> which isn't in POSIX.
>
> While many shells do have the 'local' keyword, its behaviour isn't
> portable across those either, which is why it's likely it'll never
> be added to POSIX :(

Right, but I intentionally picked the form that I thought was supported
by all reasonable '/bin/sh's: 'local [name]', without any further
adornement.  For example, per <https://mywiki.wooledge.org/Bashism>:

| 'local' is mandated by the LSB and Debian policy specifications, though only the 'local varname' (not 'local var=value') syntax is specified.

Portable, reliable shell programming is a nice idea, but then, reality
check...

(Don't ask me how much time I already spent on this simple script, to get
it into its current form -- and I'd consider myself well-versed in shell
programming...)

I was inclined to just rewrite it in Python, what do you think?  In my
opinion, a GCC-build-time Python dependency is not a problem for
'--target=nvptx-none', as that one's not in the bootstrapping chain?


Grüße
 Thomas
Sam James Dec. 9, 2024, 2:52 a.m. UTC | #4 
Thomas Schwinge <tschwinge@baylibre.com> writes:

> Hi Sam!

Hi!
>
> On 2024-12-06T09:34:32+0000, Sam James <sam@gentoo.org> wrote:
>> The script has #!/bin/sh shebang (and hence must have POSIX shell
>> compatibility), but the patch introduces uses of the 'local' keyword
>> which isn't in POSIX.
>>
>> While many shells do have the 'local' keyword, its behaviour isn't
>> portable across those either, which is why it's likely it'll never
>> be added to POSIX :(
>
> Right, but I intentionally picked the form that I thought was supported
> by all reasonable '/bin/sh's: 'local [name]', without any further
> adornement.  For example, per <https://mywiki.wooledge.org/Bashism>:
>
> | 'local' is mandated by the LSB and Debian policy specifications, though only the 'local varname' (not 'local var=value') syntax is specified.
>
> Portable, reliable shell programming is a nice idea, but then, reality
> check...

Yeah, that's fair enough. (I just noticed by chance as I was looking at
my other issue; I didn't hit an actual problem with this at all.)

>
> (Don't ask me how much time I already spent on this simple script, to get
> it into its current form -- and I'd consider myself well-versed in shell
> programming...)
>
> I was inclined to just rewrite it in Python, what do you think?  In my
> opinion, a GCC-build-time Python dependency is not a problem for
> '--target=nvptx-none', as that one's not in the bootstrapping chain?

I think that should be fine and far less error prone. Certainly not a
problem for us and I can't imagine it is for the others.

>
>
> Grüße
>  Thomas

thanks,
sam
diff mbox series

Patch

From ccd6ec23177f7a4ed69fabad8e79d5d4da419fb2 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tschwinge@baylibre.com>
Date: Mon, 2 Dec 2024 16:50:16 +0100
Subject: [PATCH] 'gcc/config/nvptx/gen-multilib-matches.sh': Support
 '--selftest'

..., and invoke that before actual use.

	gcc/
	* config/nvptx/gen-multilib-matches.sh: Support '--selftest'.
	* config/nvptx/t-nvptx (t-nvptx-gen-multilib-matches:): Invoke it.
	* config/nvptx/gen-multilib-matches-tests: New.
---
 gcc/config/nvptx/gen-multilib-matches-tests | 77 +++++++++++++++++++
 gcc/config/nvptx/gen-multilib-matches.sh    | 82 ++++++++++++++++++++-
 gcc/config/nvptx/t-nvptx                    |  2 +
 3 files changed, 159 insertions(+), 2 deletions(-)
 create mode 100644 gcc/config/nvptx/gen-multilib-matches-tests

diff --git a/gcc/config/nvptx/gen-multilib-matches-tests b/gcc/config/nvptx/gen-multilib-matches-tests
new file mode 100644
index 000000000000..c2775f268354
--- /dev/null
+++ b/gcc/config/nvptx/gen-multilib-matches-tests
@@ -0,0 +1,77 @@ 
+# Test cases for 'gen-multilib-matches.sh'.
+
+# Blank lines and lines beginning with '#' are ignored.
+
+# 'BEGIN [name]': clear state, begin test [name].
+# 'SSMS 30 35 53': set 'sms' to '30 35 53'.  Default: per 'nvptx-sm.def'.
+# 'SMOID sm_30': set 'multilib_options_isa_default' to 'sm_30'.  Default: unset.
+# 'SMOIL sm_35 sm_30': set 'multilib_options_isa_list' to 'sm_35 sm_30'.  Default: unset.
+# 'AEMM .=misa?sm_30': append '.=misa?sm_30' to expected "multilib matches".  Default: unset.
+# 'CMMC': compute "multilib matches" per the current settings, and compare to the expected.
+
+
+BEGIN '--with-arch=sm_30'
+SMOID sm_30
+SMOIL sm_30
+AEMM .=misa?sm_30
+AEMM .=misa?sm_35
+AEMM .=misa?sm_53
+AEMM .=misa?sm_70
+AEMM .=misa?sm_75
+AEMM .=misa?sm_80
+CMMC
+
+
+BEGIN '--with-arch=sm_35'
+SMOID sm_35
+SMOIL sm_35 sm_30
+AEMM .=misa?sm_35
+AEMM .=misa?sm_53
+AEMM .=misa?sm_70
+AEMM .=misa?sm_75
+AEMM .=misa?sm_80
+CMMC
+
+
+BEGIN '--with-arch=sm_53'
+SMOID sm_53
+SMOIL sm_53 sm_30
+AEMM misa?sm_30=misa?sm_35
+AEMM .=misa?sm_53
+AEMM .=misa?sm_70
+AEMM .=misa?sm_75
+AEMM .=misa?sm_80
+CMMC
+
+
+BEGIN '--with-arch=sm_70'
+SMOID sm_70
+SMOIL sm_70 sm_30
+AEMM misa?sm_30=misa?sm_35
+AEMM misa?sm_30=misa?sm_53
+AEMM .=misa?sm_70
+AEMM .=misa?sm_75
+AEMM .=misa?sm_80
+CMMC
+
+
+BEGIN '--with-arch=sm_75'
+SMOID sm_75
+SMOIL sm_75 sm_30
+AEMM misa?sm_30=misa?sm_35
+AEMM misa?sm_30=misa?sm_53
+AEMM misa?sm_30=misa?sm_70
+AEMM .=misa?sm_75
+AEMM .=misa?sm_80
+CMMC
+
+
+BEGIN '--with-arch=sm_80'
+SMOID sm_80
+SMOIL sm_80 sm_30
+AEMM misa?sm_30=misa?sm_35
+AEMM misa?sm_30=misa?sm_53
+AEMM misa?sm_30=misa?sm_70
+AEMM misa?sm_30=misa?sm_75
+AEMM .=misa?sm_80
+CMMC
diff --git a/gcc/config/nvptx/gen-multilib-matches.sh b/gcc/config/nvptx/gen-multilib-matches.sh
index 09761a9e6907..f6f2ed079f68 100755
--- a/gcc/config/nvptx/gen-multilib-matches.sh
+++ b/gcc/config/nvptx/gen-multilib-matches.sh
@@ -27,6 +27,7 @@  nvptx_dir=$(dirname "$0")
 
 
 nvptx_sm_def="$nvptx_dir/nvptx-sm.def"
+gen_multilib_matches_tests="$nvptx_dir/gen-multilib-matches-tests"
 
 sms=$(grep ^NVPTX_SM $nvptx_sm_def | sed 's/.*(//;s/,.*//')
 
@@ -88,5 +89,82 @@  print_multilib_matches() {
     echo "$multilib_matches"
 }
 
-multilib_matches=$(print_multilib_matches "$sms" "$@")
-echo "multilib_matches := $multilib_matches"
+
+selftest() {
+    [ $# = 0 ]
+
+    local sms_default
+    sms_default=$sms
+
+    local name
+    unset name
+    local sms
+    unset sms
+    local multilib_options_isa_default
+    unset multilib_options_isa_default
+    local multilib_options_isa_list
+    unset multilib_options_isa_list
+    local multilib_matches_expected
+    unset multilib_matches_expected
+
+    local line
+    line=0
+    local f1 f2
+    unset f1 f2
+    while read -r f1 f2; do
+	line=$((line + 1))
+	case "$f1 $f2" in
+	    ' ' | '#'* )
+		:
+		;;
+	    'BEGIN '* )
+		name=$f2
+		sms=$sms_default
+		unset multilib_options_isa_default
+		unset multilib_options_isa_list
+		unset multilib_matches_expected
+		;;
+	    'SSMS '* )
+		sms=$f2
+		;;
+	    'SMOID '* )
+		multilib_options_isa_default=$f2
+		;;
+	    'SMOIL '* )
+		multilib_options_isa_list=$f2
+		;;
+	    'AEMM '* )
+		multilib_matches_expected="$multilib_matches_expected $f2"
+		;;
+	    'CMMC ' )
+		local multilib_matches
+		multilib_matches=$(print_multilib_matches "${sms?}" "${multilib_options_isa_default?}" "${multilib_options_isa_list?}")
+		if [ "$multilib_matches" = "$multilib_matches_expected" ]; then
+		    echo >&2 "$0": selftest PASS "${name?}" at "$gen_multilib_matches_tests:$line"
+		else
+		    echo >&2 "$0": selftest FAIL "${name?}" at "$gen_multilib_matches_tests:$line"
+		    echo >&2 expected:"$multilib_matches_expected"
+		    echo >&2 actual:"$multilib_matches"
+		    exit 1
+		fi
+		;;
+	    * )
+		echo >&2 "$0": selftest ERROR at "$gen_multilib_matches_tests:$line"
+		echo >&2 invalid directive: "$f1 $f2"
+		exit 1
+		;;
+	esac
+    done < "$gen_multilib_matches_tests"
+}
+
+
+case "${1?}" in
+    --selftest )
+	shift
+	selftest "$@"
+	:;;
+    * )
+	multilib_matches=$(print_multilib_matches "$sms" "$@")
+	echo "multilib_matches := $multilib_matches"
+	;;
+esac
diff --git a/gcc/config/nvptx/t-nvptx b/gcc/config/nvptx/t-nvptx
index 00a7b15496e0..563c7b30dab2 100644
--- a/gcc/config/nvptx/t-nvptx
+++ b/gcc/config/nvptx/t-nvptx
@@ -48,6 +48,8 @@  t-nvptx-gen-multilib-matches: $(srcdir)/config/nvptx/gen-multilib-matches.sh \
   $(srcdir)/config/nvptx/t-nvptx \
   Makefile \
   $(srcdir)/config/nvptx/nvptx-sm.def
+	$(SHELL) $< \
+	  --selftest
 	$(SHELL) $< \
 	  $(multilib_options_isa_default) \
 	  '$(multilib_options_isa_list)' \
-- 
2.34.1