Re: [PATCH] Add a new baseboard for BPF

On 1/26/26 07:24, Piyush Raj wrote:
> The new bpf.exp baseboard is used by GCC for runtime testing of
> BPF programs across different kernel versions.

First, we will definitely need copyright papers to accept this patch.  
Please contact <[email protected]> for the details. Since you have included 
an FSF copyright notice in the proposed file, it seems clear that you 
have the intent, but the FSF needs the papers to formalize that intent 
and ensure that copyright notice is valid.

Second, I have a few preliminary review notes interspersed below.  They 
range from trivial to potentially serious on 
now-unusual-but-once-universal configurations.  Based on this 
preliminary review, I am expecting at least two to three versions of 
this patch before it can be merged.  (This initial patch counts as one 
of those.)

> 2026-01-26  Piyush Raj <[email protected]>
>      * baseboards/bpf.exp: Add a new baseboard for runtime testing
> 	  of bpf programs in gcc testsuite.
>      * Makefile.am (baseboard_DATA): Add bpf.exp.
>      * Makefile.in (baseboard_DATA): Likewise.
>
> ---
>   ChangeLog          |   6 ++
>   Makefile.am        |   1 +
>   Makefile.in        |   1 +
>   baseboards/bpf.exp | 151 +++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 159 insertions(+)
>   create mode 100644 baseboards/bpf.exp
>
> diff --git a/ChangeLog b/ChangeLog
> index 235125f..d101b51 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2026-01-26  Piyush Raj <[email protected]>
> +    * baseboards/bpf.exp: Add a new baseboard for runtime testing
> +	  of bpf programs in gcc testsuite.
> +    * Makefile.am (baseboard_DATA): Add bpf.exp.
> +    * Makefile.in (baseboard_DATA): Likewise.
> +

The convention in the DejaGnu ChangeLog is to have a blank line both 
before and after each block of changes.  Please align this with the 
other entries in the file.  Also, "gcc" here should probably be "GCC", 
referring to the package instead of the "gcc" program itself.  (I can 
fix these if needed.)

>   2025-12-02  Jacob Bachmeyer  <[email protected]>
>   
>   	* baseboards/h8300.exp: Restore from Git history per a request
> diff --git a/Makefile.am b/Makefile.am
> index 9493cc6..4ef26d1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -98,6 +98,7 @@ baseboard_DATA = \
>   	baseboards/arm-sim.exp \
>   	baseboards/basic-sid.exp \
>   	baseboards/basic-sim.exp \
> +	baseboards/bpf.exp \
>   	baseboards/cris-sim.exp \
>   	baseboards/d30v-sim.exp \
>   	baseboards/fr30-sim.exp \
> diff --git a/Makefile.in b/Makefile.in
> index 1fb4120..1e51ca8 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -469,6 +469,7 @@ baseboard_DATA = \
>   	baseboards/arm-sim.exp \
>   	baseboards/basic-sid.exp \
>   	baseboards/basic-sim.exp \
> +	baseboards/bpf.exp \
>   	baseboards/cris-sim.exp \
>   	baseboards/d30v-sim.exp \
>   	baseboards/fr30-sim.exp \
> diff --git a/baseboards/bpf.exp b/baseboards/bpf.exp
> new file mode 100644
> index 0000000..e9b98d6
> --- /dev/null
> +++ b/baseboards/bpf.exp
> @@ -0,0 +1,151 @@
> +# Copyright (C) 1997-2026, 2026 Free Software Foundation, Inc.
> +#
> +# This file is part of DejaGnu.
> +#
> +# DejaGnu is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# DejaGnu is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with DejaGnu.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Baseboard for runtime testing of BPF programs in the GCC testsuite.
> +# Uses bpf-vmtest-tool(https://gcc.gnu.org/wiki/BPFRunTimeTests)
> +
> +# Load procedures from common libraries.
> +load_lib "standard.exp"
> +# list of toolchains that are supported on this board.
> +set_board_info target_install {bpf-unknown-none}
> +set_board_info generic_name bpf
> +# bpf does not need -lm
> +set_board_info mathlib ""
> +# set board as local
> +unset_board_info isremote
> +set_board_info isremote "0"

Why is this here?  Is it absolutely logically impossible to test BPF 
remotely, or could a user possibly want to run the tests on a remote 
target or using a compiler on a remote host?  Remember that the DejaGnu 
framework supports testing in "Canadian cross" configurations where 
build, host, and target are all different architectures.

> +
> +if {[getenv VMTEST_DIR] == ""} {

This can go either way, but you can test if getenv returns an empty 
string or directly test if ::env(VMTEST_DIR) exists.  That test would be 
"if {![info exists ::env(VMTEST_DIR)]} ..." which would more closely 
match the others, but would also be less visually distinctive that 
VMTEST_DIR is an environment variable while the others are Tcl globals.

> +    verbose "ERROR: VMTEST_DIR needs to be set for bpf-vmtest-tool" 0
> +    exit 1
> +}
> +
> +if {![info exists KERNEL_VERSION]} {
> +    set KERNEL_VERSION 6.15
> +    verbose -log "Using default kernel version: $KERNEL_VERSION" 1
> +}
> +
> +if {![info exists LOG_LEVEL]} {
> +    set LOG_LEVEL ERROR
> +    verbose -log "Using default log level: $LOG_LEVEL" 1
> +}
> +
> +proc bpf_compile {source destfile type options} {
> +
> +    if { $type == "executable" } {
> +        return [bpf_target_compile $source $destfile $type $options]
> +    }
> +    return [default_target_compile $source $destfile $type $options]
> +}
Again, why is this here?  Why is a special procedure needed only to 
compile executables?
> +
> +proc bpf_target_compile {source destfile type options} {
> +
> +    global srcdir
> +    global KERNEL_VERSION
> +    global LOG_LEVEL
> +    set python3 [find_python3]
> +    if {$python3 == ""} {
> +        error "Cannot find python3 in $PATH"
> +    }

Normally, tools used in DejaGnu are referenced using global variables 
with all-caps names that are set during initialization. Here, there 
should be a "global PYTHON3" and that variable should be defined outside 
of any procedure.

> +    set script_path "$srcdir/../../contrib/bpf-vmtest-tool/main.py"

This needs to be generalized.  Remember that this is going into the 
*framework*, not just the GCC testsuite.  It is acceptable to require a 
special tool, and to default to the location in the GCC source tree, but 
the user needs to be able to override that location.  This could be as 
simple as a BPF_VMTEST_PY global variable.

> +    set opts ""
> +    foreach i $options {
> +        if {[regexp "^compiler=" $i]} {
> +            regsub "^compiler=" $i "" tmp
> +            if { $tmp eq "" } {
> +                return "bpf_target_compile: No compiler to compile with"
> +            }
> +            set compiler [ lindex [split $tmp " "] 0 ]
> +            set compiler_flags [ lindex [split $tmp " "] 1 ]
> +            append opts " $compiler_flags"
> +        }
> +
> +        if {[regexp "^additional_flags=" $i]} {
> +            regsub "^additional_flags=" $i "" tmp
> +            set tmp [string trim $tmp]
> +            append opts " $tmp"
> +        }
> +    }
> +
> +    set opts [string trim $opts]
> +
> +    setenv BPF_CFLAGS $opts
> +    setenv BPF_CC $compiler
> +    set args [list $script_path --log-level $LOG_LEVEL bpf compile $source -k $KERNEL_VERSION -o $destfile ]
> +    set status [remote_exec host $python3 $args]
> +    set exit_code [lindex $status 0]
> +    set output [lindex $status 1]
> +    unsetenv BPF_CFLAGS
> +    unsetenv BPF_CC

I will consider this acceptable, but the preferred way to do this is to 
use env(1), rather than modifying and reverting the parent's environment.

> +
> +    if { $exit_code < 0 } {
> +        verbose -log "Couldn't compile $source: $output"
> +        return $output
> +    }
> +
> +    verbose -log "Executed $source, exit_code $exit_code"
> +    if {$output ne ""} {
> +        verbose -log -- $output 2
> +    }
> +
> +    return $output
> +}

This is not acceptable.  The target_compile procedure (which will call 
bpf_compile, which will call bpf_target_compile) is documented as only 
*compiling* the program, not also running it. All targets need to be 
consistent on this.

Either the final verbose message is misleading, or this needs to be 
redesigned.

> +
> +proc bpf_load {dest prog args} {
> +    global srcdir
> +    global KERNEL_VERSION
> +    global LOG_LEVEL
> +    # Construct command
> +    set python3 [find_python3]
> +    if {$python3 == ""} {
> +        error "Cannot find python3 in $PATH"
> +    }

Again, get python3 from a global PYTHON3 that was set and checked during 
initialization.  Do not repeat the search for every testcase.  Modern 
computers are fast, but that is no excuse to be wasteful.

> +    set script_path "$srcdir/../../contrib/bpf-vmtest-tool/main.py"

Again, this needs to be customizable.

> +    set bpf_object $prog
> +
> +    set args [list $script_path --log-level $LOG_LEVEL vmtest -k $KERNEL_VERSION --bpf-obj $prog ]
> +    set status [remote_exec $dest $python3 $args]
> +    set exit_code [lindex $status 0]
> +    set output [lindex $status 1]
> +
> +    if { $exit_code < 0 } {
> +        verbose -log "Couldn't execute $prog: $output"
> +        return "unresolved"
> +    }
> +
> +    verbose -log "Executed $prog, exit_code $exit_code"
> +    if {$output ne ""} {
> +        verbose -log -- $output 2
> +    }
> +
> +    if { $exit_code == 0 } {
> +        return [list "pass" $output]
> +    } else {
> +        return [list "fail" $output]
> +    }
> +}
> +
> +
> +proc find_python3 {} {
> +    foreach candidate {python3 python} {
> +        set path [which $candidate]
> +        if {$path != ""} {
> +            return $path
> +        }
> +    }
> +    return ""
> +}

This is inadequate because it makes no attempt to verify that "python" 
is actually Python 3.  If it is intended to specifically locate a Python 
3 interpreter, it needs to ensure that it has *actually* found a Python 
3 interpreter.  You have the full reach of Expect available if needed, 
so there is no "it is too hard" excuse here.  How does the user 
determine the version of a Python interpreter?

Also, this find_python3 procedure should be located near the top of the 
file, before the bpf_* procedures, and followed by using it to set the 
PYTHON3 global variable ("set PYTHON3 [find_python3]") and then ensuring 
that PYTHON3 is set to a meaningful value, as you do for KERNEL_VERSION, 
LOG_LEVEL, and VMTEST_DIR.

Lastly, the file should probably be "bpf-vmtest.exp" since that is the 
actual target.


-- Jacob