| Message ID | 20240517114506.1259203-2-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | selftests: harness: refactor __constructor_order | expand |
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.
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) {
__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(-)