Hi Jacob,
I’ve completed the assignment process with [email protected]
Thanks for the helpful comments, I’ve prepared a v2 of the patch and
will send the new version in a separate thread.
On 27/01/26 09:32, Jacob Bachmeyer wrote:
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.)
Thanks, I have made the change for this in the new revision
[...]
+# 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.
I added that to make remote_exec work. I have now switched to using
local_exec instead to remove the hack. But that means that the board
does not support remote targets. For adding the support for remote hosts
I was not able to come up with a way (or I am understanding things
wrong) to allow the baseboard to be used locally on it own and
customizable by the user to specify remote hosts if needed.
If you have any references I could look at, that would be great!
+
+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.
I’ve implemented your suggestion to check whether the environment
variable exists and added comments to clarify the distinction between
environment and Tcl variables.
+ 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?
It was originally implemented this way because I had added support for
compiling with kernel-specific headers only for object files. I’ve now
fixed this in the new revision of the tool and the baseboard, so that it
properly supports all gcc testsuite "do-what-keyword"
+
+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.
Implemented in the latest version.
+ 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.
Implemented this as well
+ 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.
Using env(1) is a better approach, implemented this as well.
+
+ 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.
The message was misleading, I have fixed this as well. Thanks for
catching it
+
+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?
I had assumed most systems would have python3 installed and only checked
for the binary name. I’ve improved the proc in the new revision
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.
I’ve renamed the file in the new version as well
-- Jacob
Thanks
Piyush
