Re: [PING^4 PATCH 0/1] sim/testsuite: PR29596, Fix broken "make check-sim" by trimming excess path from arch

[Andrew Burgess <aburgess@redhat.com>]

Hi!

Thanks for looking at this issue.


Tsukasa OI <research_trasio@irq.a4lg.com> writes:

> TODO
> NOTE: the cover letter and subject may change each time I ping.

For a single patch series like this a cover letter really isn't needed.
All the information you've written in this cover letter would be better
included in the commit message for the patch.  It's all really useful
info that somebody looking at this patch in the future might want to
know.

>
> Hello,
>
> This patch fixes the problem which make check-sim (testing the simulator)
> doesn't work on many targets.
>
> The patch is a PING (4) of
> <https://sourceware.org/pipermail/binutils/2022-July/121814.html>
> <https://sourceware.org/pipermail/gdb-patches/2022-August/191564.html>.
> Tracker on GitHub:
> <https://github.com/a4lg/binutils-gdb/wiki/sim_trim_path_from_arch>
>
> In this ping, I removed ChangeLog and won't change anymore (unless I miss
> something).
>
>
>
> I first reproduced this bug while I'm adding simulator tests to RISC-V
> but talking with AArch64 (with some upstream tests) would be better.
>
>
>
> [How to reproduce]
>
> 1.  Configure Binutils with aarch64-unknown-elf and build it
>     /src/binutils/configure --target=aarch64-unknown-elf && make
> 2.  Run `make check-sim' and confirmed that the simulator tests "pass"
> 3.  Intentionally try to fail the test by modifying
>     `sim/testsuite/aarch64/pass.s'
>     (replace the last line from "pass" to "fail")
> 4.  Run `make check-sim' and "confirmed" that the simulator tests "pass"
>     (it should fail!)

I followed these instructions but I couldn't reproduce the problem you
are seeing.  More below...

>
>
>
> [Analysis]
>
> ... Yes, something is going wrong.  After the test, you can see the test
> log in `sim/testsuite/aarch64/allinsn/testrun.log':
>
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> Test run by builduser on Sat Aug 27 01:04:31 2022
> Target is aarch64-unknown-elf
> Host   is x86_64-pc-linux-gnu
>
>                 ===  tests ===
>
> Schedule of variations:
>     unix
>
> Running target unix
> Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
> Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
> Using /src/binutils/sim/testsuite/config/default.exp as tool-and-target-specific interface file.
> WARNING: Assuming target board is the local machine (which is probably wrong).
> You may need to set your DEJAGNU environment variable.
> Running /src/binutils/sim/testsuite/./aarch64/allinsn.exp ...

I don't see this, I see:

  Running ../../src.dev-1/sim/testsuite/aarch64/allinsn.exp ...

> Can't find a compatible assembler
> Executing on host: false /src/binutils/sim/testsuite/lib/compilercheck.c     -E -o testsuite/./aarch64/allinsn/compilercheck.x    (timeout = 300)
> spawn -ignore SIGHUP false /src/binutils/sim/testsuite/lib/compilercheck.c -E -o testsuite/./aarch64/allinsn/compilercheck.x
> compiler exited with status 1
> Can't find a compatible C compiler
> Testing adds.s on machine aarch64.
> UNTESTED: ./aarch64/adds.s
> Testing addv.s on machine aarch64.
> UNTESTED: ./aarch64/addv.s
> [...]
> Testing pass.s on machine aarch64.
> UNTESTED: ./aarch64/pass.s

And here I see:

  Testing pass.s on machine aarch64.
  Executing on host: /tmp/build/./gas/as-new ../../src/sim/testsuite/aarch64/pass.s  -I../../src/sim/testsuite/aarch64  -o /tmp/build/sim/pass.s.o    (timeout = 300)
  spawn -ignore SIGHUP /tmp/build/./gas/as-new ../../src/sim/testsuite/aarch64/pass.s -I../../src/sim/testsuite/aarch64 -o /tmp/build/sim/pass.s.o
  Executing on host: /tmp/build/./ld/ld-new /tmp/build/sim/pass.s.o   -o /tmp/build/sim/pass.s.x    (timeout = 300)
  spawn -ignore SIGHUP /tmp/build/./ld/ld-new /tmp/build/sim/pass.s.o -o /tmp/build/sim/pass.s.x
  ./aarch64/run    /tmp/build/sim/pass.s.x 
  spawn ./aarch64/run /tmp/build/sim/pass.s.x
  fail

which is what we'd expect.

I'm using commit d2d69057a2c, which is current HEAD of master.

Any idea why I might be seeing different results to you?

Thanks,
Andrew

> [...]
> Testing xtn.s on machine aarch64.
> UNTESTED: ./aarch64/xtn.s
> testcase /src/binutils/sim/testsuite/./aarch64/allinsn.exp completed in 0 seconds
>
>                 ===  Summary ===
>
> # of untested testcases         27
> runtest completed at Sat Aug 27 01:04:31 2022
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
>
> We clearly find that the test runner didn't find recently built assembler
> (gas/as-new).  That assembler (and the linker) is supposed to be used
> because the simulator itself (sim/Makefile) sets its configuration.
>
> Quoting `sim/site.exp' and `sim/site-sim-config.exp':
>     set AS_FOR_TARGET_AARCH64 "/build/binutils/./gas/as-new"
>     set LD_FOR_TARGET_AARCH64 "/build/binutils/./ld/ld-new"
>
>
> At last, we find `sim/testsuite/lib/sim-defs.exp'.
>
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> proc sim_init_toolchain {} {
> [...]
>     set arch [sim_arch]
>     set ARCH [string map {- _} [string toupper $arch]]
>     foreach var {AS LD CC} {
>     set var_for_target "${var}_FOR_TARGET"
>     global $var_for_target
>     set var_for_target_arch "${var_for_target}_${ARCH}"
>     global $var_for_target_arch
>
>     if [info exists $var_for_target_arch] {
>         set $var_for_target [set $var_for_target_arch]
>     } else {
>         set $var_for_target ""
>     }
> [...]
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
>
> In here, it extracts {AS,LD,CC}_FOR_TARGET_AARCH64 and sets proper
> {AS,LD,CC}_FOR_TARGET.  At least, it is supposed to do so.
>
> However, this block doesn't work because the `arch' variable returned by
> the `sim_arch' function is "./aarch64".  That is supposed to be "aarch64".
>
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> proc sim_arch {} {
>     global subdir
>     set arch "$subdir"
>     while { [file dirname $arch] != "." } {
>     set arch [file dirname $arch]
>     }
>     return "$arch"
> }
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
>
>
> [Fix: Cause and Patch Details]
>
> `sim_arch' function is simple.  Until the dirname part of the "arch" is not
> "." (current directory), it trims the filename part.
>
> For instance, if subdir is "A/B/C/D", this function returns "A".  However,
> if subdir is "./A/B/C/D", this function returns "./A", not "A".
> In fact, actual subdir value here is "./aarch64" and we will get
> "./aarch64" for [sim_arch].  As a result, it fails to extract proper
> assembler/linker/compiler for given target.
>
> To deal with it, I added another "file tail" function call before returning.
>
>
>
> [Analysis: After the Patch]
>
> After this patch, `sim/testsuite/aarch64/allinsn/testrun.log' will
> look like this:
>
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> Test run by builduser on Sat Aug 27 01:34:29 2022
> Target is aarch64-unknown-elf
> Host   is x86_64-pc-linux-gnu
>
>                 ===  tests ===
>
> Schedule of variations:
>     unix
>
> Running target unix
> Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
> Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
> Using /src/binutils/sim/testsuite/config/default.exp as tool-and-target-specific interface file.
> WARNING: Assuming target board is the local machine (which is probably wrong).
> You may need to set your DEJAGNU environment variable.
> Running /src/binutils/sim/testsuite/./aarch64/allinsn.exp ...
> Found a compatible assembler
> Can't execute C compiler
> Testing adds.s on machine aarch64.
> Executing on host: /build/binutils/./gas/as-new /src/binutils/sim/testsuite/./aarch64/adds.s  -I/src/binutils/sim/testsuite/./aarch64  -o testsuite/./aarch64/allinsn/adds.s.o    (timeout = 300)
> spawn -ignore SIGHUP /build/binutils/./gas/as-new /src/binutils/sim/testsuite/./aarch64/adds.s -I/src/binutils/sim/testsuite/./aarch64 -o testsuite/./aarch64/allinsn/adds.s.o
> Executing on host: /build/binutils/./ld/ld-new testsuite/./aarch64/allinsn/adds.s.o   -o testsuite/./aarch64/allinsn/adds.s.x    (timeout = 300)
> spawn -ignore SIGHUP /build/binutils/./ld/ld-new testsuite/./aarch64/allinsn/adds.s.o -o testsuite/./aarch64/allinsn/adds.s.x
> ./aarch64/run    testsuite/./aarch64/allinsn/adds.s.x
> spawn ./aarch64/run testsuite/./aarch64/allinsn/adds.s.x
> pass
> PASS: aarch64 adds.s
> [...]
> Testing pass.s on machine aarch64.
> Executing on host: /build/binutils/./gas/as-new /src/binutils/sim/testsuite/./aarch64/pass.s  -I/src/binutils/sim/testsuite/./aarch64  -o testsuite/./aarch64/allinsn/pass.s.o    (timeout = 300)
> spawn -ignore SIGHUP /build/binutils/./gas/as-new /src/binutils/sim/testsuite/./aarch64/pass.s -I/src/binutils/sim/testsuite/./aarch64 -o testsuite/./aarch64/allinsn/pass.s.o
> Executing on host: /build/binutils/./ld/ld-new testsuite/./aarch64/allinsn/pass.s.o   -o testsuite/./aarch64/allinsn/pass.s.x    (timeout = 300)
> spawn -ignore SIGHUP /build/binutils/./ld/ld-new testsuite/./aarch64/allinsn/pass.s.o -o testsuite/./aarch64/allinsn/pass.s.x
> ./aarch64/run    testsuite/./aarch64/allinsn/pass.s.x
> spawn ./aarch64/run testsuite/./aarch64/allinsn/pass.s.x
> fail
> status:  0
> output:  fail
>
> pattern: pass
>
> FAIL: aarch64 pass.s (execution)
> [...]
> Testing xtn.s on machine aarch64.
> Executing on host: /build/binutils/./gas/as-new /src/binutils/sim/testsuite/./aarch64/xtn.s  -I/src/binutils/sim/testsuite/./aarch64  -o testsuite/./aarch64/allinsn/xtn.s.o    (timeout = 300)
> spawn -ignore SIGHUP /build/binutils/./gas/as-new /src/binutils/sim/testsuite/./aarch64/xtn.s -I/src/binutils/sim/testsuite/./aarch64 -o testsuite/./aarch64/allinsn/xtn.s.o
> Executing on host: /build/binutils/./ld/ld-new testsuite/./aarch64/allinsn/xtn.s.o   -o testsuite/./aarch64/allinsn/xtn.s.x    (timeout = 300)
> spawn -ignore SIGHUP /build/binutils/./ld/ld-new testsuite/./aarch64/allinsn/xtn.s.o -o testsuite/./aarch64/allinsn/xtn.s.x
> ./aarch64/run    testsuite/./aarch64/allinsn/xtn.s.x
> spawn ./aarch64/run testsuite/./aarch64/allinsn/xtn.s.x
> pass
> PASS: aarch64 xtn.s
> testcase /src/binutils/sim/testsuite/./aarch64/allinsn.exp completed in 0 seconds
>
>                 ===  Summary ===
>
> # of expected passes            26
> # of unexpected failures        1
> runtest completed at Sat Aug 27 01:34:29 2022
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
>
> OK, we "successfully" failed.  If we restore `sim/testsuite/aarch64/pass.s',
> it completely succeeds.
>
> This patch trims "./" part of "./arch" using the `file tail' function.
> Even if the subdir input is "aarch64", it returns "aarch64".
> With this patch, we can now successfully test the simulator.
>
>
> Regards,
> Tsukasa
>
>
>
>
> Tsukasa OI (1):
>   sim/testsuite: PR29596, Trim extra path from arch
>
>  sim/testsuite/lib/sim-defs.exp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> base-commit: c10a862f17847bc9c50d680c87b87dc51ae4b95e
> -- 
> 2.34.1