Re: [LTP] [PATCH 1/2] generate_arch.sh: Fix syscall extraction bugs

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Some improvements below. Feel free to ignore them and I'll send a following
patchset afterwards.
...
> -grab_syscall_names_from_unistd_h() {
>  	grep -E -h "^#define __NR_" \
> -		${LINUX_HEADERS}/usr/include/asm/unistd*.h \
> -		${LINUX_HEADERS}/usr/include/asm-generic/unistd.h \
> -		>${TEMP}/syscall-names.tosort
> +		${KERNELSRC}/include/uapi/asm-generic/unistd.h \
> +		${KERNELSRC}/arch/*/include/uapi/asm/unistd*.h \
Is the star (*) intentional? It will work, but IMHO it's not needed.

> +		>>${TEMP}/syscall-names.tosort 2>/dev/null || true

Redirecting stderr to /dev/null is effective way to hide bugs forever.
I suppose the reason here is to hide error that some file is missing.
But I'd prefer to see the "No such file or directory" errors than hide a bug,
could you remove "2>/dev/null" before the release?

Also "|| true" used due the same reason will hide any other error.
Are really some files missing? Even if yes, wouldn't be better to before loop
over the files, test if they exist with [ -f ... ] and store existing ones in
variable which you pass grep.

> +
> +	awk '{ print $1 }' ${SCRIPT_DIR}/*.in >>${TEMP}/syscall-names.tosort

>  	drop_bad_entries
>  }
> @@ -60,6 +59,7 @@ grab_syscall_names_from_unistd_h() {
>  drop_bad_entries() {
>  	grep -E -v "(unistd.h|NR3264|__NR_syscall|__SC_COMP|__NR_.*Linux|__NR_FAST)" \
>  		${TEMP}/syscall-names.tosort |
> +		grep -E -vi "(not implemented|available|unused|reserved|spill|xtensa)" |
>  		grep -E -v "(__SYSCALL|SYSCALL_BASE|SYSCALL_MASK)" |
>  		sed -e "s/#define\s*__NR_//g" -e "s/\s.*//g" |
>  		sort -u >${TEMP}/syscall-names.txt
> @@ -68,51 +68,43 @@ drop_bad_entries() {
>  generate_table() {
>  	echo "- $arch"

> +	local flags="${extraflags:-}"
>  	if [ "$bits" -eq "32" ]; then
> -		extraflags="${extraflags} -D__BITS_PER_LONG=32"
> +		flags="${flags} -D__BITS_PER_LONG=32"
>  	fi

>  	local uppercase_arch=$(echo "$arch" | tr '[:lower:]' '[:upper:]')

> -	# ignore any error generated by gcc. We want to obtain all the
> -	# available architecture syscalls for the current platform and to handle
> -	# only supported architectures later on
> -	gcc ${TEMP}/list-syscalls.c -U__LP64__ -U__ILP32__ -U__i386__ \
> +	rm -f "${TEMP}/list-syscalls"
> +	gcc "${TEMP}/list-syscalls.c" -U__LP64__ -U__ILP32__ -U__i386__ \
>  		-D${uppercase_arch} \
> -		-D__${arch}__ ${extraflags} \
> -		-I ${LINUX_HEADERS}/usr/include/ \
> -		-o ${TEMP}/list-syscalls || true
> +		-D__${arch}__ ${flags} \
> +		-I "${LINUX_HEADERS}/usr/include/" \
> +		-o "${TEMP}/list-syscalls" || {
> +		echo "Failed to compile list-syscalls for $arch" >&2
> +		if grep -Fxq "$arch" "${SUPPORTED_ARCH}"; then
> +			return 1
> +		fi
> +		return 0
> +	}

> -	${TEMP}/list-syscalls >"${TEMP}/${arch}.in.tosort"
> +	"${TEMP}/list-syscalls" >"${TEMP}/${arch}.in.tosort"

>  	sort -k2,2n "${TEMP}/${arch}.in.tosort" >"${TEMP}/${arch}.in"
>  }

>  generate_list_syscalls_c() {
> -	(
> -		printf "
> -		#include <stdio.h>
> -		#include <asm/unistd.h>
> -
> -		int main(void)
> -		{
> -		"
> -		for syscall in $(cat ${TEMP}/syscall-names.txt); do
> -			printf "
> -		#ifdef __NR_$syscall
> -			printf(\"$syscall %%d"
> -			# i know the following print is ugly, but dash and bash
> -			# treat double quoted strings in a different way and we
> -			# really need to inject '\n' character in the C code
> -			# rather than carriage return
> -			printf '\\n'
> -			printf "\", __NR_$syscall);
> -		#endif
> -		"
> -		done
> -		printf " return 0;
> -		}"
> -	) >${TEMP}/list-syscalls.c
> +	{
> +		echo "#include <stdio.h>"
> +		echo "#include <asm/unistd.h>"
> +		echo
> +		echo "int main(void)"
> +		echo "{"

I suppose you prefer many echo calls, but using cat is a standard way which is
IMHO more readable (specially if you store awk output into a variable so that
you can use single cat).

	cat << EOF
#include <stdio.h>
#include <asm/unistd.h>

int main(void)
{
EOF

> +		awk '{ printf "#ifdef __NR_%s\n\tprintf(\"%s %%d\\n\", __NR_%s);\n#endif\n", $1, $1, $1 }' \
> +			"${TEMP}/syscall-names.txt"
> +		echo "	return 0;"
> +		echo "}"
> +	} >"${TEMP}/list-syscalls.c"
>  }

>  export_headers() {
> @@ -128,16 +120,16 @@ do_all_tables() {
>  		extraflags=

>  		case ${arch} in
> -		Kconfig)
> -			continue
> -			;;
> -		um)
> +		Kconfig|um)

+1, at least newer model can write a decent code.

I still consider it brave to have long shell script (specially if one generates
them with LLM) :).

>  			continue
>  			;;
>  		esac

> +		if [ ! -f "${archdir}/Makefile" ] && [ ! -f "${archdir}/Kconfig" ]; then
I would check only for Kconfig, that's enough (there is no arch which would not
have it).

> +			continue
> +		fi
> +

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp