Re: [PATCH v5 03/13] scripts/syscall.tbl: Use the common table

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] POSIX shell syntax violation (`bashism`) causes build failure.
- [High] Sequential table processing breaks strict monotonicity check, preventing architectures from adding new local syscalls.
- [High] Sequential table processing silently truncates `__NR_syscalls`.
- [Medium] Tooling Breakage: `tools/perf` fails to parse common syscalls for generic architectures.
- [Medium] Missing Make dependency for `syscall_common.tbl` breaks incremental builds.
--

--- Patch [3]: [PATCH v5 03/13] scripts/syscall.tbl: Use the common table ---
commit e407e18fccaf1e26e59809a57f9bedc05e1b414e
Author: André Almeida <[email protected]>

scripts/syscall.tbl: Use the common table

This commit removes duplicated code by moving common syscall numbers out of
arch/arm64/tools/syscall_32.tbl and scripts/syscall.tbl to a shared common
table.

> diff --git a/scripts/Makefile.asm-headers b/scripts/Makefile.asm-headers
> index 8a4856e741803..681713f240cc1 100644
> --- a/scripts/Makefile.asm-headers
> +++ b/scripts/Makefile.asm-headers
> @@ -15,9 +15,10 @@ all:
>  
>  src := $(srctree)/$(subst /generated,,$(obj))
>  
> -syscall_abis_32  += common,32
> -syscall_abis_64  += common,64
> +syscall_abis_32  += clone3,common,32
> +syscall_abis_64  += clone3,common,64
>  syscalltbl := $(srctree)/scripts/syscall.tbl
> +syscalltbl_common := $(srctree)/scripts/syscall_common.tbl

[Severity: Medium]
Since syscalltbl_common is introduced here and passed to the generator scripts,
should it also be added to the Make prerequisites?

Currently, targets like unistd_%.h only depend on $(syscalltbl) and $(syshdr):

$(obj)/unistd_%.h: $(syscalltbl) $(syshdr) FORCE
	$(call if_changed,syshdr)

Without adding $(syscalltbl_common) to these dependencies, Kbuild won't trigger
a rebuild of the headers when the common table changes, which can break
incremental builds.

>  syshdr-args := --emit-nr
>  
>  # let architectures override $(syscall_abis_%) and $(syscalltbl)

[ ... ]

> @@ -58,14 +59,14 @@ quiet_cmd_remove = REMOVE  $(unwanted)
>        cmd_remove = rm -f $(unwanted)
>  
>  quiet_cmd_syshdr = SYSHDR  $@
> -      cmd_syshdr = $(CONFIG_SHELL) $(syshdr) \
> +      cmd_syshdr = $(CONFIG_SHELL) $(syshdr) --common-tbl $(syscalltbl_common) \

[Severity: High]
Does calling syscallhdr.sh here break on systems where $(CONFIG_SHELL) is dash
(like Debian/Ubuntu)?

scripts/syscallhdr.sh uses a #!/bin/sh shebang but contains bash-specific
process substitution:

    done < <(grep -E "^[0-9A-Fa-fXx]+[[:space:]]+$abis" "$input")

On POSIX-compliant shells, this causes a syntax error ('redirection
unexpected') and aborts the kernel build.

Additionally, does passing --common-tbl silently truncate __NR_syscalls?

When syscallhdr.sh processes the local table first, the global "max" variable
tracks the highest local syscall. But when it processes the common table
afterward, it unconditionally overwrites it:

    max=$nr

If an architecture has a local syscall higher than the common table's highest
syscall, max gets incorrectly capped. This results in an incorrect
__NR_syscalls value.

>  		   $(if $(syshdr-args-$*),$(syshdr-args-$*),$(syshdr-args)) \
>  		   $(if $(syscall_compat),--prefix "compat$*_") \
>  		   --abis $(subst $(space),$(comma),$(strip $(syscall_abis_$*))) \
>  		   $< $@
>  
>  quiet_cmd_systbl = SYSTBL  $@
> -      cmd_systbl = $(CONFIG_SHELL) $(systbl) \
> +      cmd_systbl = $(CONFIG_SHELL) $(systbl) --common-tbl $(syscalltbl_common) \

[Severity: High]
Similar to the syshdr script, syscalltbl.sh also contains a bashism:

    done < <(grep -E "^[0-9]+[[:space:]]+$abis" "$input")

Will this sequential processing also break strict monotonicity checks for
architectures adding new local syscalls?

syscalltbl.sh uses a global "nxt" variable to ensure numerical sorting.
If an architecture adds a local syscall > 403 (e.g., 472), "nxt" advances
to 473. When it then processes the common table starting at 403, the check:

    if [ $nxt -gt $nr ]; then
        echo "error: $input: syscall table is not sorted..."

will trigger (473 > 403) and abort the build.

>  		   $(if $(systbl-args-$*),$(systbl-args-$*),$(systbl-args)) \
>  		   --abis $(subst $(space),$(comma),$(strip $(syscall_abis_$*))) \
>  		   $< $@

[ ... ]

> diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> index 7a42b32b65776..ee828d03094ed 100644
> --- a/scripts/syscall.tbl
> +++ b/scripts/syscall.tbl
> @@ -344,71 +344,3 @@
>  293	common	rseq				sys_rseq
>  294	common	kexec_file_load			sys_kexec_file_load
>  # 295 through 402 are unassigned to sync up with generic numbers don't use
> -403	32	clock_gettime64			sys_clock_gettime

[Severity: Medium]
Does removing syscalls 403 through 471 from this file break perf trace?

The perf tooling parses this file directly to generate syscall name mappings:

tools/perf/trace/beauty/syscalltbl.sh:
    build_tables "$tools_dir/scripts/syscall.tbl" "$outfile"

Since tools/perf/trace/beauty/syscalltbl.sh isn't updated to parse the new
syscall_common.tbl, perf trace will lose name resolution for syscalls >= 403
on architectures using the generic table.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3