Skip to content

Remove RV32 qemu test scripts from upstream scope - #552

Closed
Winstonllllai wants to merge 2 commits into
eclipse-threadx:devfrom
Winstonllllai:rv32-remove-qemu-test-scripts
Closed

Winstonllllai wants to merge 2 commits into
eclipse-threadx:devfrom
Winstonllllai:rv32-remove-qemu-test-scripts

Conversation

@Winstonllllai

Copy link
Copy Markdown
Contributor

Summary

Remove the local-only QEMU/GDB test script artifacts from the RV32 port.
This is a follow-up cleanup after review feedback on the earlier RV32 work. These scripts were useful for local validation, but they are not needed in the upstream PR scope.

Drop the RV32 qemu_virt CMake integration, Python/GDB test script, and
test-only demo changes so the port no longer carries local-only QEMU
validation artifacts in the upstream tree.
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for following up on @akifejaz's request so promptly. Removing test/threadx_test_tx_gnu_riscv32_qemu.py and the check-functional-riscv32 target is right, and it matches what he asked for on #549.

Two things go further than that, though, and I would like to separate them out. I tested both rather than reasoning from the diff, with riscv32-unknown-elf-gcc and qemu-system-riscv32.

The boot pinning is load bearing — please keep it

The PR reverts entry.S from .section .text.boot, "ax" to .section .text, and drops this from link.lds:

KEEP(*(.text.boot))   /* entry.s _start — must be first at 0x80000000 */

That comment is accurate. QEMU's virt machine with -bios none begins executing at a fixed 0x80000000, so _start has to be the first thing in the image, whatever the ELF entry point says.

With the PR applied the demo still boots — but only by accident, because build_libthreadx.sh happens to list entry.S first on the compiler command line. Move it later in that list and the linker places _start elsewhere:

build _start boots under QEMU
dev, entry.S first 0x80000000 yes, threads scheduling
dev, entry.S last 0x80000000 yes — pinning holds it
this PR, entry.S first 0x80000000 yes
this PR, entry.S last 0x80000d80 no output at all

In the last case the CPU jumps to 0x80000000 and runs whatever function the linker happened to put there. KEEP(*(.text.boot)) is what makes the boot address independent of source order, so it is a genuine fix rather than test scaffolding. Please restore both halves.

Deleting the whole CMakeLists loses the demo build target

qemu_virt/CMakeLists.txt does two jobs: it builds kernel.elf, and it registers the check-functional-riscv32 runner. Only the second was objected to. Deleting the file removes both, and unregistering EXAMPLE_DIR in ports/risc-v32/gnu/CMakeLists.txt removes the last CMake route to the example.

build_libthreadx.sh still builds and runs it, so nothing is stranded, but that script starts with rm -rf ../../../../../build/ and hardcodes its flags — it is a local convenience, not a build system. The rest of the tree has been moving toward CMake for exactly these examples, so I would keep the add_executable(kernel.elf ...) block and delete only the find_package(Python3) / add_custom_target(check-functional-riscv32 ...) section at the bottom.

One process note

This targets master. Please retarget it to dev, which is where the other RISC-V work has been landing.

Removing demo_threadx.c's test hooks and the .py script is exactly right — it is just the boot fix and the demo target that should survive the cleanup.

@fdesbiens
fdesbiens changed the base branch from master to dev August 18, 2026 19:49
The previous commit went beyond removing the test runner: it also dropped
the .text.boot pinning that keeps _start at 0x80000000 regardless of source
order, and deleted the whole example CMakeLists instead of only the
check-functional-riscv32 target.
@Winstonllllai

Copy link
Copy Markdown
Contributor Author

Hi @fdesbiens ,

Thanks for testing both scenarios rather than just reasoning from the diff — that boot-order dependency would have been very easy to miss. I've addressed both points.

Boot pinning restored
Reverted entry.S back to .section .text.boot, "ax", and restored the KEEP(*(.text.boot)) line in link.lds. Agreed this is a genuine fix, not test scaffolding — it shouldn't have been swept up in the cleanup. _start is now pinned at 0x80000000 independent of source/link order again.

CMakeLists.txt build target restored
Kept qemu_virt/CMakeLists.txt and the add_executable(kernel.elf ...) block, along with the EXAMPLE_DIR registration in ports/risc-v32/gnu/CMakeLists.txt. Removed only the find_package(Python3) call and the add_custom_target(check-functional-riscv32 ...) block, so the demo still has a CMake build path — just without the test runner.

Base branch
Saw you already retargeted this to dev — no action needed on my end there.

The PR should now be scoped to exactly what was asked: test script and demo test-hooks removed, boot pinning and demo build target kept. Let me know if anything else needs adjusting.

@fdesbiens

Copy link
Copy Markdown
Contributor

Closing this one — not because anything in it was wrong, but because the ground it stood on has moved.

What changed underneath it

This removes test/threadx_test_tx_gnu_riscv32_qemu.py and the check-functional-riscv32 target, following @akifejaz's June request on #549 to keep the QEMU runners as local tooling.

He has since taken the opposite view in practice: #691 rebuilt that same runner and wired the CMake target up properly. It is now the functional verification for the RISC-V32 port — MEPC integrity, time-slice handling, system clock, preemption, and a soft-float skip path — and #698 did the same for RISC-V64. Both landed this week.

So merging this now would delete test infrastructure that was rewritten and merged days ago. Git says as much on its own: the branch no longer merges, and the conflict is modify/delete — deleted in HEAD and modified in origin/dev.

None of that reflects on your work. You did what the review asked, promptly, and the request was reasonable when it was made.

The two points from the earlier review

Both are now moot, for the same reason:

  • The boot pinning. I asked for .text.boot and KEEP(*(.text.boot)) to be restored, because without them _start lands at 0x80000000 only by accident of source order. That pinning is on dev and staying.
  • The add_executable(kernel.elf ...) block. riscv32: spec compliance and regression test fix #691 rewrote qemu_virt/CMakeLists.txt around it, adding GDB and QEMU discovery and the --skip-fpu wiring for soft-float builds. There is no longer a version of this change that keeps the demo target and drops only the runner.

Where your contribution did land

Your other pull request, #549, is going in. The lazy FPU stacking merged cleanly with #698's rewrite of the same files, your mstatus.FS initialisation in the stack build is in, and your new-thread FPU state test is now the 96th test in the RISC-V64 regression suite. I checked that it fails when the behaviour it guards is broken, which is the part that makes a test worth having.

Thanks for the cleanup work here regardless — it was the right response to the review you were given.

@fdesbiens fdesbiens closed this Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants