Usage of longjmp() was added to ensure that teardown is always run in
commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN for ASSERT failures")
However instead of calling longjmp() to the teardown handler it is easier to
just call the teardown handler directly from __bail().
Any potential duplicate teardown invocations are harmless as the actual
handler will only ever be executed once since
commit fff37bd32c76 ("selftests/harness: Fix fixture teardown").
Additionally this removes a incompatibility with nolibc,
which does not support setjmp()/longjmp().
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
tools/testing/selftests/kselftest_harness.h | 45 ++++++++++-------------------
1 file changed, 15 insertions(+), 30 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 088c875df91a58f8760749b6047b246fb2a7891f..2925e47db995d7197ed1f55bd9cc657669df9bd3 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -67,7 +67,6 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
-#include <setjmp.h>
#include "kselftest.h"
@@ -178,9 +177,7 @@
struct __test_metadata *_metadata, \
struct __fixture_variant_metadata __attribute__((unused)) *variant) \
{ \
- if (setjmp(_metadata->env) == 0) \
- test_name(_metadata); \
- __test_check_assert(_metadata); \
+ test_name(_metadata); \
} \
static struct __test_metadata _##test_name##_object = \
{ .name = #test_name, \
@@ -425,24 +422,20 @@
} \
_metadata->variant = variant->data; \
_metadata->self = self; \
- if (setjmp(_metadata->env) == 0) { \
- /* _metadata and potentially self are shared with all forks. */ \
- child = fork(); \
- if (child == 0) { \
- fixture_name##_setup(_metadata, self, variant->data); \
- /* Let setup failure terminate early. */ \
- if (_metadata->exit_code) \
- _exit(0); \
- *_metadata->no_teardown = false; \
- fixture_name##_##test_name(_metadata, self, variant->data); \
- } else if (child < 0 || child != waitpid(child, &status, 0)) { \
- ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
- _metadata->exit_code = KSFT_FAIL; \
- } \
- } \
+ /* _metadata and potentially self are shared with all forks. */ \
+ child = fork(); \
if (child == 0) { \
+ fixture_name##_setup(_metadata, self, variant->data); \
+ /* Let setup failure terminate early. */ \
+ if (_metadata->exit_code) \
+ _exit(0); \
+ *_metadata->no_teardown = false; \
+ fixture_name##_##test_name(_metadata, self, variant->data); \
_metadata->teardown_fn(false, _metadata, self, variant->data); \
_exit(0); \
+ } else if (child < 0 || child != waitpid(child, &status, 0)) { \
+ ksft_print_msg("ERROR SPAWNING TEST GRANDCHILD\n"); \
+ _metadata->exit_code = KSFT_FAIL; \
} \
_metadata->teardown_fn(true, _metadata, self, variant->data); \
munmap(_metadata->no_teardown, sizeof(*_metadata->no_teardown)); \
@@ -456,7 +449,6 @@
/* Forward signal to __wait_for_test(). */ \
kill(getpid(), WTERMSIG(status)); \
} \
- __test_check_assert(_metadata); \
} \
static void wrapper_##fixture_name##_##test_name##_teardown( \
bool in_parent, struct __test_metadata *_metadata, \
@@ -927,7 +919,6 @@ struct __test_metadata {
int timeout; /* seconds to wait for test timeout */
bool aborted; /* stopped test due to failed ASSERT */
bool *no_teardown; /* fixture needs teardown */
- jmp_buf env; /* for exiting out of test early */
void *self;
const void *variant;
struct __test_results *results;
@@ -963,19 +954,14 @@ static inline int __bail(int for_realz, struct __test_metadata *t)
{
/* if this is ASSERT, return immediately. */
if (for_realz) {
- t->aborted = true;
- longjmp(t->env, 1);
+ if (t->teardown_fn)
+ t->teardown_fn(false, t, t->self, t->variant);
+ abort();
}
/* otherwise, end the for loop and continue. */
return 0;
}
-static inline void __test_check_assert(struct __test_metadata *t)
-{
- if (t->aborted)
- abort();
-}
-
static void __wait_for_test(struct __test_metadata *t)
{
/*
@@ -1208,7 +1194,6 @@ static void __run_test(struct __fixture_metadata *f,
t->trigger = 0;
t->aborted = false;
t->no_teardown = NULL;
- memset(t->env, 0, sizeof(t->env));
memset(t->results->reason, 0, sizeof(t->results->reason));
snprintf(test_name, sizeof(test_name), "%s%s%s.%s",
--
2.49.0
Hi Thomas,
CC += Jason
On Mon, May 05, 2025 at 05:15:30PM +0200, Thomas Weißschuh wrote:
> Usage of longjmp() was added to ensure that teardown is always run in
> commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN for ASSERT failures")
> However instead of calling longjmp() to the teardown handler it is easier to
> just call the teardown handler directly from __bail().
> Any potential duplicate teardown invocations are harmless as the actual
> handler will only ever be executed once since
> commit fff37bd32c76 ("selftests/harness: Fix fixture teardown").
>
> Additionally this removes a incompatibility with nolibc,
> which does not support setjmp()/longjmp().
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
The iommufd selftest (CONFIG_IOMMUFD_TEST) starts to give warnings
when building with v6.16-rc1, though the test code wasn't changed
at these two functions:
------------------------------------------------------------------
make: Entering directory '/nicolinc/linux-stable/tools/testing/selftests/iommu'
CC iommufd
iommufd.c: In function ‘wrapper_iommufd_mock_domain_all_aligns’:
iommufd.c:1806:17: warning: ‘mfd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
1806 | close(mfd);
| ^~~~~~~~~~
iommufd.c:1766:13: note: ‘mfd’ was declared here
1766 | int mfd;
| ^~~
iommufd.c: In function ‘wrapper_iommufd_mock_domain_all_aligns_copy’:
iommufd.c:1869:17: warning: ‘mfd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
1869 | close(mfd);
| ^~~~~~~~~~
iommufd.c:1818:13: note: ‘mfd’ was declared here
1818 | int mfd;
| ^~~
CC iommufd_fail_nth
make: Leaving directory '/nicolinc/linux-stable/tools/testing/selftests/iommu'
------------------------------------------------------------------
Git bisect points to this patch, and reverting it fixes these.
Both mfds are under the same "if (variant->driver)" check, so the
warnings don't seem legit to me.
Do you have any idea why this happens?
Thanks
Nicolin
Hi Nicolin,
On Mon, Jun 09, 2025 at 11:40:34PM -0700, Nicolin Chen wrote:
> Hi Thomas,
>
> CC += Jason
>
> On Mon, May 05, 2025 at 05:15:30PM +0200, Thomas Weißschuh wrote:
> > Usage of longjmp() was added to ensure that teardown is always run in
> > commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN for ASSERT failures")
> > However instead of calling longjmp() to the teardown handler it is easier to
> > just call the teardown handler directly from __bail().
> > Any potential duplicate teardown invocations are harmless as the actual
> > handler will only ever be executed once since
> > commit fff37bd32c76 ("selftests/harness: Fix fixture teardown").
> >
> > Additionally this removes a incompatibility with nolibc,
> > which does not support setjmp()/longjmp().
> >
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>
> The iommufd selftest (CONFIG_IOMMUFD_TEST) starts to give warnings
> when building with v6.16-rc1, though the test code wasn't changed
> at these two functions:
Thanks for the report.
> ------------------------------------------------------------------
> make: Entering directory '/nicolinc/linux-stable/tools/testing/selftests/iommu'
> CC iommufd
> iommufd.c: In function ‘wrapper_iommufd_mock_domain_all_aligns’:
> iommufd.c:1806:17: warning: ‘mfd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 1806 | close(mfd);
> | ^~~~~~~~~~
> iommufd.c:1766:13: note: ‘mfd’ was declared here
> 1766 | int mfd;
> | ^~~
> iommufd.c: In function ‘wrapper_iommufd_mock_domain_all_aligns_copy’:
> iommufd.c:1869:17: warning: ‘mfd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 1869 | close(mfd);
> | ^~~~~~~~~~
> iommufd.c:1818:13: note: ‘mfd’ was declared here
> 1818 | int mfd;
> | ^~~
> CC iommufd_fail_nth
> make: Leaving directory '/nicolinc/linux-stable/tools/testing/selftests/iommu'
> ------------------------------------------------------------------
>
> Git bisect points to this patch, and reverting it fixes these.
>
> Both mfds are under the same "if (variant->driver)" check, so the
> warnings don't seem legit to me.
>
> Do you have any idea why this happens?
It does look very weird. I could understand if the compiler assumes that
variant->file changes during the runtime of the function.
But even if I work around this, by introducing a local variable "bool file =
variant->file" the issue persists.
However as soon as the value of of "bool file" is fixed to either "true" or
"false" it goes away.
The following diff *alone* also prevents the warning, but that doesn't
make any sense either:
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -65,7 +65,7 @@ static inline void *memfd_mmap(size_t length, int prot, int flags, int *mfd_p)
return MAP_FAILED;
if (ftruncate(mfd, length))
return MAP_FAILED;
- *mfd_p = mfd;
+ *mfd_p = 0;
return mmap(0, length, prot, flags, mfd, 0);
}
Maybe the logic became too complex for GCC?
Case in point, when trying with an older GCC 13.2, the following warning appeared:
In file included from iommufd_utils.h:14,
from iommufd.c:12:
In function 'iommufd_viommu_vdevice_alloc',
inlined from 'wrapper_iommufd_viommu_vdevice_alloc' at iommufd.c:2731:1:
../kselftest_harness.h:760:12: warning: 'ret' may be used uninitialized [-Wmaybe-uninitialized]
760 | if (!(__exp _t __seen)) { \
| ^
../kselftest_harness.h:513:9: note: in expansion of macro '__EXPECT'
513 | __EXPECT(expected, #expected, seen, #seen, ==, 1)
| ^~~~~~~~
iommufd_utils.h:1005:9: note: in expansion of macro 'ASSERT_EQ'
1005 | ASSERT_EQ(0, _test_cmd_trigger_vevents(self->fd, dev_id, nvevents))
| ^~~~~~~~~
iommufd.c:2766:17: note: in expansion of macro 'test_cmd_trigger_vevents'
2766 | test_cmd_trigger_vevents(dev_id, 3);
| ^~~~~~~~~~~~~~~~~~~~~~~~
iommufd_utils.h: In function 'wrapper_iommufd_viommu_vdevice_alloc':
iommufd_utils.h:993:13: note: 'ret' was declared here
993 | int ret;
| ^~~
Which is also just a false-positive and fixed with newer GCCs.
Thomas
On Tue, Jun 10, 2025 at 02:21:25PM +0200, Thomas Weißschuh wrote:
> The following diff *alone* also prevents the warning, but that doesn't
> make any sense either:
>
> --- a/tools/testing/selftests/iommu/iommufd_utils.h
> +++ b/tools/testing/selftests/iommu/iommufd_utils.h
> @@ -65,7 +65,7 @@ static inline void *memfd_mmap(size_t length, int prot, int flags, int *mfd_p)
> return MAP_FAILED;
> if (ftruncate(mfd, length))
> return MAP_FAILED;
> - *mfd_p = mfd;
> + *mfd_p = 0;
> return mmap(0, length, prot, flags, mfd, 0);
> }
>
>
> Maybe the logic became too complex for GCC?
Maybe. Those warnings are gone using a dummy setjmp() without a
longjmp() :-/
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 2925e47db995..2dc288413fc7 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -67,6 +67,7 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <setjmp.h>
#include "kselftest.h"
@@ -407,6 +408,7 @@
FIXTURE_DATA(fixture_name) self_private, *self = NULL; \
pid_t child = 1; \
int status = 0; \
+ jmp_buf test = {}; \
/* Makes sure there is only one teardown, even when child forks again. */ \
_metadata->no_teardown = mmap(NULL, sizeof(*_metadata->no_teardown), \
PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); \
@@ -422,6 +424,7 @@
} \
_metadata->variant = variant->data; \
_metadata->self = self; \
+ setjmp(test); \
/* _metadata and potentially self are shared with all forks. */ \
child = fork(); \
if (child == 0) { \
© 2016 - 2025 Red Hat, Inc.