Covered the trace entry update paths and the misaligned stack adjustment - #666
Merged
fdesbiens merged 1 commit intoAug 26, 2026
Merged
Conversation
Against the merged coverage report -- every build configuration instrumented and
unioned -- eighteen lines of common/src were uncovered. Seventeen of them were
one missing scenario rather than eighteen separate gaps.
tx_block_allocate, tx_byte_allocate, tx_thread_system_suspend and
tx_thread_system_resume each carry blocks under TX_ENABLE_EVENT_TRACE that go
back and patch a trace entry once the call has done its work, all of the shape:
if (entry_ptr != TX_NULL)
{
if (time_stamp == entry_ptr -> tx_trace_buffer_entry_time_stamp)
entry_ptr comes from _tx_trace_buffer_current_ptr, which stays TX_NULL until
tx_trace_enable is called at run time. Building with TX_ENABLE_EVENT_TRACE is
not enough, and exactly one test in the suite enables tracing --
threadx_trace_basic_test -- which tests the enable API itself and never calls
either allocator. So those blocks sat in the report's denominator and never in
its covered set.
threadx_trace_entry_update_test enables tracing and then drives both allocators
twice each, once on the path that succeeds immediately and once through a
suspension that a second thread satisfies, since each allocator carries one
update block on either side. It then sleeps so that the last runnable thread
suspends with nothing ready to take over: tx_thread_system_suspend lines 345 and
351 are on the branch that sets _tx_thread_execute_ptr to TX_NULL, and the two
allocator suspensions never reach it because the other thread was always ready.
The same test closes tx_trace_object_register's TX_NULL name break by creating a
semaphore with no name. A semaphore and not a thread deliberately: for
TX_TRACE_OBJECT_TYPE_THREAD the register function dereferences the pointer it is
given to read the thread's priority, so that type needs a real TX_THREAD behind
it. threadx_trace_basic_test makes the equivalent call only under
ifndef TX_ENABLE_EVENT_TRACE, against the no-op stub.
threadx_thread_misaligned_stack_test covers the remaining line,
tx_thread_create.c:136, where a stack that does not begin on a ULONG boundary
costs a ULONG of size so that rounding the start up cannot run past the end of
the caller's buffer. Every other test hands tx_thread_create an aligned stack.
That line is compiled only under TX_ENABLE_STACK_CHECKING, so it is absent from
three of the five configurations' reports rather than uncovered in them, and it
was verified under stack_checking_build.
Measured on the merged report, all 480 tests passing and 5 of 5 configurations
green: 4485 of 4503 covered before, 4502 of 4503 after, denominator unchanged.
One line remains, tx_thread_system_resume.c:529, and it is the report's last
flapping line rather than a standing gap -- two clean runs of the same tree gave
4503 of 4503 and 4502 of 4503. Reaching it by construction was tried twice and
failed both times, so it is left alone here. It needs the preempt disable flag
and the system state both clear, and tx_thread_resume raises the preempt disable
flag before calling _tx_thread_system_resume, as do the put and send paths;
creating a higher priority auto-start thread from thread context does not raise
it but does not reach the check either, which a probe showed is executed only
during initialisation, with the system state at TX_INITIALIZE_IN_PROGRESS.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
fdesbiens
added a commit
that referenced
this pull request
Aug 26, 2026
The coverage summary reported a percentage and could not fail. Coverage could fall from 99.97% to anything at all and every check stayed green, against an AGENTS.md that asks for 100% test coverage -- a stated requirement measured with a gauge that had no failure mode. CodeCoverageSummary already takes thresholds and fail_below_min; neither was set. Both are now, through a new coverage_thresholds input on the template, because the two suites do not sit at the same figure: ThreadX 99, SMP 98. Three things were probed against the pinned action on a runner before picking those numbers, using the real merged.xml files from the dev push run of #666. The floor compares the line rate and nothing else. That mattered because branch coverage is around 78% in both suites while line coverage is 98.8-100%, so a floor aimed at the line figure would have been an immediate red wall had it tested branches or the lower of the two. The ThreadX report at 100.00% lines and 77.67% branches clears a floor of 99. The thresholds are whole numbers. '99.9 100' -- the value this was meant to be -- is rejected with 'System.ArgumentException - Threshold parameter set incorrectly.', and the step fails whether or not fail_below_min is set. So the choice is 99 or 100 with nothing between. 100 would fail on a race. tx_thread_system_resume.c:529 is reached by timing rather than by construction and flaps between runs of the same green tree, which is why #666 left it; 4502/4503 fails a floor of 100 and clears one of 99. A coverage gate that goes red on a coin toss is how coverage gates get switched off. SMP is 5114/5178 lines, 98.76%, with 64 uncovered lines across 11 files of common_smp/src -- #666 closed the equivalent gaps in common/src only. A shared floor of 99 would have failed that job on every run while ThreadX passed. One limit is recorded in the file rather than fixed: an empty report reads as 100%. gcovr writes line-rate="1.0" beside lines-valid="0" when it finds no data, and the action prints 'Line Rate = 100% (0 / 0)' and passes any floor. The check for that is the emptiness assertion #664 put in each suite's coverage.sh, not this one. Also corrected two stale filenames in the deploy job's comment: since #665 each coverage artifact carries merged.xml, not default_build_coverage.xml. Verified on the runner. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Against the merged coverage report — every build configuration instrumented and unioned, which is #665 — eighteen lines of
common/srcwere uncovered. Seventeen of them were one missing scenario rather than eighteen separate gaps.The scenario
tx_block_allocate.c,tx_byte_allocate.c,tx_thread_system_suspend.candtx_thread_system_resume.ceach carry blocks underTX_ENABLE_EVENT_TRACEthat go back and patch a trace entry once the call has done its work, all of the same shape:entry_ptrcomes from_tx_trace_buffer_current_ptr, which staysTX_NULLuntiltx_trace_enableis called at run time. Building withTX_ENABLE_EVENT_TRACEis not enough, and exactly one test in the suite enables tracing —threadx_trace_basic_test— which tests the enable API itself and never calls either allocator. So those blocks sat in the report's denominator and never in its covered set.The tests
threadx_trace_entry_update_testenables tracing and then drives both allocators twice each — once on the path that succeeds immediately and once through a suspension that a second thread satisfies — because each allocator carries one update block on either side.It then sleeps so the last runnable thread suspends with nothing ready to take over. That is not padding:
tx_thread_system_suspend.c345 and 351 are on the branch that sets_tx_thread_execute_ptrtoTX_NULL, and the two allocator suspensions never reach it because the other thread was always ready.The same test closes
tx_trace_object_register'sTX_NULL-namebreakby creating a semaphore with no name. A semaphore and not a thread deliberately: forTX_TRACE_OBJECT_TYPE_THREADthe register function dereferences the pointer it is handed to read the thread's priority, so that type needs a realTX_THREADbehind it.threadx_thread_misaligned_stack_testcovers the remaining line,tx_thread_create.c:136, where a stack that does not begin on aULONGboundary costs aULONGof size so that rounding the start up cannot run past the end of the caller's buffer. Every other test handstx_thread_createan aligned stack. That line is compiled only underTX_ENABLE_STACK_CHECKING, so it is absent from three of the five configurations' reports rather than uncovered in them — it was verified understack_checking_build.Measured
All 480 tests passing, 5 of 5 configurations green:
The denominator is unchanged, as it should be — these are tests, not new kernel code.
One line remains, and it is a flapping line rather than a gap
tx_thread_system_resume.c:529. Two clean runs of the same tree, both green, gave 4503/4503 and 4502/4503 — so a floor set at 100% would fail on a race.Reaching it by construction was tried twice and failed both times, so it is left alone here. It needs
_tx_thread_preempt_disableand_tx_thread_system_stateboth clear at that point:tx_thread_resumeraises the preempt disable flag before calling_tx_thread_system_resume, and so do the put and send paths.TX_INITIALIZE_IN_PROGRESS.The test written for the second attempt was deleted rather than kept: it passed and exercised real behaviour, but its documented purpose was covering that line and it did not.
Note for review
The before/after figures are against the merged report from #665. These tests stand on their own and pass in every configuration without it — but without #665 only
default_build_coverageis instrumented, and most of what they cover is not measured.