This series refactors __constructor_order because __constructor_order_last() is unneeded.
BTW, the comments in kselftest_harness.h was confusing to me.
As far as I tested, all arches executed constructors in the forward order.
[test code]
#include <stdio.h>
static int x;
static void __attribute__((constructor)) increment(void) { x += 1; }
static void __attribute__((constructor)) multiply(void) { x *= 2; }
int main(void) { printf("foo = %d\n", x); return 0; }
It should print 2 for forward order systems, 1 for reverse order systems.
I executed it on some archtes by using QEMU. I always got 2.
Masahiro Yamada (2): selftests: harness: remove unneeded __constructor_order_last() selftests: harness: rename __constructor_order for clarification
.../drivers/s390x/uvdevice/test_uvdevice.c | 6 ------ tools/testing/selftests/hid/hid_bpf.c | 6 ------ tools/testing/selftests/kselftest_harness.h | 18 ++++-------------- tools/testing/selftests/rtc/rtctest.c | 7 ------- 4 files changed, 4 insertions(+), 33 deletions(-)
__constructor_order_last() is unneeded.
If __constructor_order_last() is not called on reverse-order systems, __constructor_order will remain 0 instead of being set to _CONSTRUCTOR_ORDER_BACKWARD (= -1).
__LIST_APPEND() will still take the 'else' branch, so there is no difference in the behavior.
Signed-off-by: Masahiro Yamada masahiroy@kernel.org ---
.../selftests/drivers/s390x/uvdevice/test_uvdevice.c | 6 ------ tools/testing/selftests/hid/hid_bpf.c | 6 ------ tools/testing/selftests/kselftest_harness.h | 10 +--------- tools/testing/selftests/rtc/rtctest.c | 7 ------- 4 files changed, 1 insertion(+), 28 deletions(-)
diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c index ea0cdc37b44f..7ee7492138c6 100644 --- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c +++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c @@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr) att_inval_addr_test(&self->uvio_attest.meas_addr, _metadata, self); }
-static void __attribute__((constructor)) __constructor_order_last(void) -{ - if (!__constructor_order) - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; -} - int main(int argc, char **argv) { int fd = open(UV_PATH, O_ACCMODE); diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c index 2cf96f818f25..f47feef2aced 100644 --- a/tools/testing/selftests/hid/hid_bpf.c +++ b/tools/testing/selftests/hid/hid_bpf.c @@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level, return 0; }
-static void __attribute__((constructor)) __constructor_order_last(void) -{ - if (!__constructor_order) - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; -} - int main(int argc, char **argv) { /* Use libbpf 1.0 API mode */ diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index ba3ddeda24bf..60c1cf5b0f0d 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -444,12 +444,6 @@ * Use once to append a main() to the test file. */ #define TEST_HARNESS_MAIN \ - static void __attribute__((constructor)) \ - __constructor_order_last(void) \ - { \ - if (!__constructor_order) \ - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \ - } \ int main(int argc, char **argv) { \ return test_harness_run(argc, argv); \ } @@ -846,7 +840,6 @@ static struct __fixture_metadata *__fixture_list = &_fixture_global; static int __constructor_order;
#define _CONSTRUCTOR_ORDER_FORWARD 1 -#define _CONSTRUCTOR_ORDER_BACKWARD -1
static inline void __register_fixture(struct __fixture_metadata *f) { @@ -1272,8 +1265,7 @@ static int test_harness_run(int argc, char **argv)
static void __attribute__((constructor)) __constructor_order_first(void) { - if (!__constructor_order) - __constructor_order = _CONSTRUCTOR_ORDER_FORWARD; + __constructor_order = _CONSTRUCTOR_ORDER_FORWARD; }
#endif /* __KSELFTEST_HARNESS_H */ diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 63ce02d1d5cc..9647b14b47c5 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { ASSERT_EQ(new, secs); }
-static void __attribute__((constructor)) -__constructor_order_last(void) -{ - if (!__constructor_order) - __constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; -} - int main(int argc, char **argv) { switch (argc) {
On Fri, May 17, 2024 at 08:45:05PM +0900, Masahiro Yamada wrote:
__constructor_order_last() is unneeded.
If __constructor_order_last() is not called on reverse-order systems, __constructor_order will remain 0 instead of being set to _CONSTRUCTOR_ORDER_BACKWARD (= -1).
__LIST_APPEND() will still take the 'else' branch, so there is no difference in the behavior.
Signed-off-by: Masahiro Yamada masahiroy@kernel.org
.../selftests/drivers/s390x/uvdevice/test_uvdevice.c | 6 ------ tools/testing/selftests/hid/hid_bpf.c | 6 ------ tools/testing/selftests/kselftest_harness.h | 10 +--------- tools/testing/selftests/rtc/rtctest.c | 7 ------- 4 files changed, 1 insertion(+), 28 deletions(-)
diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c index ea0cdc37b44f..7ee7492138c6 100644 --- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c +++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c @@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr) att_inval_addr_test(&self->uvio_attest.meas_addr, _metadata, self); } -static void __attribute__((constructor)) __constructor_order_last(void) -{
- if (!__constructor_order)
__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;-}
int main(int argc, char **argv) { int fd = open(UV_PATH, O_ACCMODE); diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c index 2cf96f818f25..f47feef2aced 100644 --- a/tools/testing/selftests/hid/hid_bpf.c +++ b/tools/testing/selftests/hid/hid_bpf.c @@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level, return 0; } -static void __attribute__((constructor)) __constructor_order_last(void) -{
- if (!__constructor_order)
__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;-}
int main(int argc, char **argv) { /* Use libbpf 1.0 API mode */ diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index ba3ddeda24bf..60c1cf5b0f0d 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -444,12 +444,6 @@
- Use once to append a main() to the test file.
*/ #define TEST_HARNESS_MAIN \
- static void __attribute__((constructor)) \
- __constructor_order_last(void) \
- { \
if (!__constructor_order) \__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \- } \ int main(int argc, char **argv) { \ return test_harness_run(argc, argv); \ }
This won't work. All constructors are executed, so we have to figure out which is run _first_. Switching this to a boolean means we gain no information about ordering: it'll always be set to "true". We need to detect which constructor sets it first so that we can walk the lists (that are built via all the constructors in between) in the correct order.
@@ -846,7 +840,6 @@ static struct __fixture_metadata *__fixture_list = &_fixture_global; static int __constructor_order; #define _CONSTRUCTOR_ORDER_FORWARD 1 -#define _CONSTRUCTOR_ORDER_BACKWARD -1 static inline void __register_fixture(struct __fixture_metadata *f) { @@ -1272,8 +1265,7 @@ static int test_harness_run(int argc, char **argv) static void __attribute__((constructor)) __constructor_order_first(void) {
- if (!__constructor_order)
__constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
- __constructor_order = _CONSTRUCTOR_ORDER_FORWARD;
} #endif /* __KSELFTEST_HARNESS_H */ diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 63ce02d1d5cc..9647b14b47c5 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { ASSERT_EQ(new, secs); } -static void __attribute__((constructor)) -__constructor_order_last(void) -{
- if (!__constructor_order)
__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;-}
int main(int argc, char **argv) { switch (argc) {
A better question is why these tests are open-coding the execution of "main"...
On Sat, May 18, 2024 at 8:26 AM Kees Cook keescook@chromium.org wrote:
On Fri, May 17, 2024 at 08:45:05PM +0900, Masahiro Yamada wrote:
__constructor_order_last() is unneeded.
If __constructor_order_last() is not called on reverse-order systems, __constructor_order will remain 0 instead of being set to _CONSTRUCTOR_ORDER_BACKWARD (= -1).
__LIST_APPEND() will still take the 'else' branch, so there is no difference in the behavior.
Signed-off-by: Masahiro Yamada masahiroy@kernel.org
.../selftests/drivers/s390x/uvdevice/test_uvdevice.c | 6 ------ tools/testing/selftests/hid/hid_bpf.c | 6 ------ tools/testing/selftests/kselftest_harness.h | 10 +--------- tools/testing/selftests/rtc/rtctest.c | 7 ------- 4 files changed, 1 insertion(+), 28 deletions(-)
diff --git a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c index ea0cdc37b44f..7ee7492138c6 100644 --- a/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c +++ b/tools/testing/selftests/drivers/s390x/uvdevice/test_uvdevice.c @@ -257,12 +257,6 @@ TEST_F(attest_fixture, att_inval_addr) att_inval_addr_test(&self->uvio_attest.meas_addr, _metadata, self); }
-static void __attribute__((constructor)) __constructor_order_last(void) -{
if (!__constructor_order)__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;-}
int main(int argc, char **argv) { int fd = open(UV_PATH, O_ACCMODE); diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c index 2cf96f818f25..f47feef2aced 100644 --- a/tools/testing/selftests/hid/hid_bpf.c +++ b/tools/testing/selftests/hid/hid_bpf.c @@ -853,12 +853,6 @@ static int libbpf_print_fn(enum libbpf_print_level level, return 0; }
-static void __attribute__((constructor)) __constructor_order_last(void) -{
if (!__constructor_order)__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;-}
int main(int argc, char **argv) { /* Use libbpf 1.0 API mode */ diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index ba3ddeda24bf..60c1cf5b0f0d 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -444,12 +444,6 @@
- Use once to append a main() to the test file.
*/ #define TEST_HARNESS_MAIN \
static void __attribute__((constructor)) \__constructor_order_last(void) \{ \if (!__constructor_order) \__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD; \} \ int main(int argc, char **argv) { \ return test_harness_run(argc, argv); \ }This won't work. All constructors are executed, so we have to figure out which is run _first_. Switching this to a boolean means we gain no information about ordering: it'll always be set to "true".
It will be set to "true" eventually, but __LIST_APPEND() still sees "false" on backward-order systems.
Let's see how the following is expanded.
#include "kselftest_harness.h"
TEST(foo) { ... }
TEST(bar) { ... }
You will get something as follows:
void _attribute__((constructor)) __constructor_order_first(void) { __constructor_order_forward = true; }
void __attribute__((constructor)) _register_foo(void) { __register_test(&foo_object); // call __LIST_APPEND() for foo }
void __attribute__((constructor)) _register_bar(void) { __register_test(&bar_object); // call __LIST_APPEND() for bar }
On forward-order systems, the constructors are executed in this order:
__constructor_order_first() -> _register_foo() -> _register_bar()
So, __LIST_APPEND will see "true".
On backward-order systems, the constructors are executed in this order:
_register_bar() -> _register_foo() -> __constructor_order_first()
So, __LIST_APPEND will see "false" since __construtor_order_first() has not been called yet.
Correct me if I am wrong.
We need to detect which constructor sets it first so that we can walk the lists (that are built via all the constructors in between)
You have a wrong assumption here.
TEST() macros may not be placed in-between.
#include "kselftest_harness.h"
TEST_HARNESS_MAIN TEST(foo) { ... } TEST(bar) { ... }
This is perfectly correct code, because there is no reason to force "Please put TEST_HARNESS_MAIN at the end of the file".
It is just a "coding style".
If the test code were written in such style with the current harness implementation, __constructor_order would be zero instead of _CONSTRUCTOR_ORDER_BACKWARD on backward-order systems. __LIST_APPEND() still works correctly, though.
#endif /* __KSELFTEST_HARNESS_H */ diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 63ce02d1d5cc..9647b14b47c5 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -410,13 +410,6 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { ASSERT_EQ(new, secs); }
-static void __attribute__((constructor)) -__constructor_order_last(void) -{
if (!__constructor_order)__constructor_order = _CONSTRUCTOR_ORDER_BACKWARD;-}
int main(int argc, char **argv) { switch (argc) {
A better question is why these tests are open-coding the execution of "main"...
It is open __unnecessary__ coding.
If __constructor_order_last() had not existed in the first place, such things would not have occured.
-- Best Regards Masahiro Yamada
On Sat, May 18, 2024 at 12:29:00PM +0900, Masahiro Yamada wrote:
It will be set to "true" eventually, but __LIST_APPEND() still sees "false" on backward-order systems.
Ah, yes -- you are right. I looked through the commit history (I had to go back to when the seccomp test, and the harness, was out of tree). There was a time when the logic happened during the list walking, rather than during list _creation_. I was remembering the former.
So, yes, let's make this change. As you say, it also solves for defining TEST_HARNESS_MAIN before the tests. Thank you! I'd still like to replace all the open-coded TEST_HARNESS_MAIN calls, though.
Now, __constructor_order is boolean; 1 for forward-order systems, 0 for reverse-order systems while parsing __LIST_APPEND().
Change it into a bool variable, and rename it for clarification.
Signed-off-by: Masahiro Yamada masahiroy@kernel.org ---
tools/testing/selftests/kselftest_harness.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 60c1cf5b0f0d..55f96037582b 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -774,7 +774,7 @@ item->prev = item; \ return; \ } \ - if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) { \ + if (__constructor_order_forward) { \ item->next = NULL; \ item->prev = head->prev; \ item->prev->next = item; \ @@ -837,9 +837,7 @@ struct __test_xfail { }
static struct __fixture_metadata *__fixture_list = &_fixture_global; -static int __constructor_order; - -#define _CONSTRUCTOR_ORDER_FORWARD 1 +static bool __constructor_order_forward;
static inline void __register_fixture(struct __fixture_metadata *f) { @@ -891,7 +889,7 @@ static inline bool __test_passed(struct __test_metadata *metadata) * list so tests are run in source declaration order. * https://gcc.gnu.org/onlinedocs/gccint/Initialization.html * However, it seems not all toolchains do this correctly, so use - * __constructor_order to detect which direction is called first + * __constructor_order_foward to detect which direction is called first * and adjust list building logic to get things running in the right * direction. */ @@ -1265,7 +1263,7 @@ static int test_harness_run(int argc, char **argv)
static void __attribute__((constructor)) __constructor_order_first(void) { - __constructor_order = _CONSTRUCTOR_ORDER_FORWARD; + __constructor_order_forward = true; }
#endif /* __KSELFTEST_HARNESS_H */
On Fri, May 17, 2024 at 08:45:04PM +0900, Masahiro Yamada wrote:
This series refactors __constructor_order because __constructor_order_last() is unneeded.
BTW, the comments in kselftest_harness.h was confusing to me.
As far as I tested, all arches executed constructors in the forward order.
[test code]
#include <stdio.h>
static int x;
static void __attribute__((constructor)) increment(void) { x += 1; }
static void __attribute__((constructor)) multiply(void) { x *= 2; }
int main(void) { printf("foo = %d\n", x); return 0; }
It should print 2 for forward order systems, 1 for reverse order systems.
I executed it on some archtes by using QEMU. I always got 2.
IIRC, and it was a long time ago now, it was actually a difference between libc implementations where I encountered the problem. Maybe glibc vs Bionic?
-Kees
linux-kselftest-mirror@lists.linaro.org
-
Kees Cook -
Masahiro Yamada