tests/spinlock: Terminate temporary thread after tests #60123
Conversation
The spinlock tests start an additional thread to perform tests with. If the
thread isn't stopped after each test, then when the same thread structure
is used in subsequent tests, the thread structure will end up in an
inconsistent state. In this particular cases, the second instance doesn't
end up going through z_thread_entry and so z_tls_current is not set
correctly, leading to faults whenever that value is used.
This fault can be demonstrated by running the test with thread local
storage enabled:
$ twister -p qemu_cortex_a53_smp \
--test tests/kernel/spinlock/kernel.multiprocessing.spinlock \
-xCONFIG_THREAD_LOCAL_STORAGE=y
Signed-off-by: Keith Packard <[email protected]>
| @@ -137,6 +137,8 @@ ZTEST(spinlock, test_spinlock_bounce) | |||
| bounce_once(1234, false); | |||
| } | |||
|
|
|||
| k_thread_abort(&cpu1_thread); | |||
There was a problem hiding this comment.
Probably a good idea to do k_thread_join() to make sure the thread is no longer running.
There was a problem hiding this comment.
Waiting for termination is a guarantied part of abort's semantics. Abort and join actually share the same implementation.
There was a problem hiding this comment.
Good catch. This is a semi-common class of bug, actually. There was discussion a while back in the context of ztest that it would be a good feature (albeit mildly complicated) for the test framework to put a level of indirection around "worker threads" like this to ensure that they don't live beyond their test case.
Would it be reasonable to stick a check in |
Not without breaking API compatibility, unfortunately. The existing call takes a completely uninitialized thread struct. It might not be zero (or a valid terminated thread), or might have been allocated from the heap and contain previous garbage, etc... We can't do any inspection of the struct at all. It's a historical wart, basically, like the inexplicable decision to pass three separate pointer arguments. If we were to do it again at some point it would be nice to separate the "init" of the struct from the "spawn" of the thread (and also other stuff, like the pointer nonsense, or removing the timeout argument, which really only has value for static threads -- threads created at runtime can just as easily use a k_timer to do it, via a separate "spawn_thread_later()" wrapper if desired, etc...). But for the special case of tests... maybe? Have a separate rule when CONFIG_TEST=y where we enforce that it must be valid/zero? That seems weird, but doable. |
|
Fixed in #60567 |
The spinlock tests start an additional thread to perform tests with. If the thread isn't stopped after each test, then when the same thread structure is used in subsequent tests, the thread structure will end up in an inconsistent state. In this particular cases, the second instance doesn't end up going through z_thread_entry and so z_tls_current is not set correctly, leading to faults whenever that value is used.
This fault can be demonstrated by running the test with thread local storage enabled: