[LTP] [PATCH] lib: do_setup(): Check for supported arch first

[LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Cyril Hrubis]

This commit moves the check for supported architecture before the check
for test function existence. This allows us do ifdef out the run
function pointer initialization and properly TCONF on unsupported
platform.

Example usage:

 #include "tst_test.h"

 #ifdef __x86_64__
 static void run(void)
 {
	...
 }
 #endif

 struct tst_test test = {
 #ifdef __x86_64__
	.run = run,
 #endif
	.supported_archs = {"x86_64", NULL},
 }

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 lib/tst_test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 694861d95..c47ddd86a 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1273,6 +1273,9 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->tconf_msg)
 		tst_brk(TCONF, "%s", tst_test->tconf_msg);
 
+	if (tst_test->supported_archs && !tst_is_on_arch(tst_test->supported_archs))
+		tst_brk(TCONF, "This arch '%s' is not supported for test!", tst_arch.name);
+
 	assert_test_fn();
 
 	TCID = tid = get_tid(argv);
@@ -1301,9 +1304,6 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->min_kver)
 		check_kver(tst_test->min_kver, 1);
 
-	if (tst_test->supported_archs && !tst_is_on_arch(tst_test->supported_archs))
-		tst_brk(TCONF, "This arch '%s' is not supported for test!", tst_arch.name);
-
 	if (tst_test->skip_in_lockdown && tst_lockdown_enabled() > 0)
 		tst_brk(TCONF, "Kernel is locked down, skipping test");
 
-- 
2.49.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Andrea Cervesato via ltp]

Hi Cyril,

On 4/29/25 15:55, Cyril Hrubis wrote:
> This commit moves the check for supported architecture before the check
> for test function existence. This allows us do ifdef out the run
> function pointer initialization and properly TCONF on unsupported
> platform.
>
> Example usage:
>
>   #include "tst_test.h"
>
>   #ifdef __x86_64__
>   static void run(void)
>   {
> 	...
>   }
>   #endif
>
>   struct tst_test test = {
>   #ifdef __x86_64__
> 	.run = run,
>   #endif
> 	.supported_archs = {"x86_64", NULL},
>   }
>
I guess the reason is that we want to skip code which is not compiling 
for our architecture, right?
But do we already have TST_TEST_CONF, so I'm a bit puzzled on the use case.

Isn't way more easy to use:

#ifndef __x86_64__
...
#else
TST_TEST_TCONF("Test supports x86_64 arch only");
#endif

And eventually combined statements can be used, so:

#if defined __i386__ || defined(__x86_64__)
...

is still valid and it looks easier to use.

- Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Cyril Hrubis]

Hi!
> I guess the reason is that we want to skip code which is not compiling 
> for our architecture, right?
> But do we already have TST_TEST_CONF, so I'm a bit puzzled on the use case.

The problem here is that with TST_TEST_TCONF() the supported architecture
is not exported into the metadata.

There are two cases where TST_TEST_TCONF() is used in LTP:

- usupported architecture for the test
- missing headers during compilation

The first case should ideally be converted so that we have the supported
architectures in the test metadata.

Another option is just to add metadata to the tst_test structure and
keep TST_TEST_TCONF(), but I do not like that solution, because the
ifdefs and .supported_archs can go out of sync easily because tst_test
structure is not used when TST_TEST_TCONF() is used.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Andrea Cervesato via ltp]

> The problem here is that with TST_TEST_TCONF() the supported architecture
> is not exported into the metadata.
>
> There are two cases where TST_TEST_TCONF() is used in LTP:
>
> - usupported architecture for the test
> - missing headers during compilation
>
> The first case should ideally be converted so that we have the supported
> architectures in the test metadata.
>
> Another option is just to add metadata to the tst_test structure and
> keep TST_TEST_TCONF(), but I do not like that solution, because the
> ifdefs and .supported_archs can go out of sync easily because tst_test
> structure is not used when TST_TEST_TCONF() is used.
>
Ok now it's more clear thanks.
What about adding a macro like TST_TEST_TCONF that sets archs? At the 
end, that's what we actually do already in the tst_test structure for 
tconf_msg:

#define TST_TEST_TCONF(message)                                 \
         static struct tst_test test = { .tconf_msg = message  } \

Something like:

#define TST_TEST_TCONF2(message, archs)                       \
         static struct tst_test test = { .tconf_msg = message, 
.supported_archs = archs  } \

I know it's ugly the way I implemented it here, but it's just to 
understand. So we could have only one preprocessor directive checking 
for arches and TST_TEST_TCONF2 setting up the metadata anyway.
In general, I don't like adding multiple preprocessor statements inside 
tests because code would be puzzled with multiple arch definitions 
checks. Maybe you have a better idea than me for this TST_TEST_TCONF2 
implementation.

- Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Cyril Hrubis]

Hi!
> Ok now it's more clear thanks.
> What about adding a macro like TST_TEST_TCONF that sets archs? At the 
> end, that's what we actually do already in the tst_test structure for 
> tconf_msg:
> 
> #define TST_TEST_TCONF(message)                                 \
>          static struct tst_test test = { .tconf_msg = message  } \
> 
> Something like:
> 
> #define TST_TEST_TCONF2(message, archs)                       \
>          static struct tst_test test = { .tconf_msg = message, 
> .supported_archs = archs  } \

That will not get into the metadata, because the tst_test structure from
the TST_TEST_TCONF() is not parsed by the metadata extractor and that is
on purpose because the real data is in the other one.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Andrea Cervesato via ltp]

On 4/29/25 17:20, Cyril Hrubis wrote:
> That will not get into the metadata, because the tst_test structure from
> the TST_TEST_TCONF() is not parsed by the metadata extractor and that is
> on purpose because the real data is in the other one.
It seems like there's no alternative solution then. Feel free to merge 
the patch.
Thanks!

Acked-by: Andrea Cervesato <andrea.cervesato@suse.com>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Petr Vorel]

Hi Cyril,

> This commit moves the check for supported architecture before the check
> for test function existence. This allows us do ifdef out the run
> function pointer initialization and properly TCONF on unsupported
> platform.

LGTM. Ideally could you also convert some test (as example which others
can follow - like they do for docparse fixes).

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Example usage:

>  #include "tst_test.h"

>  #ifdef __x86_64__
I would personally did not bother with #ifdef, but it's true it makes resulted
binary smaller (for non-intel arch).

Kind regards,
Petr

>  static void run(void)
>  {
> 	...
>  }
>  #endif

>  struct tst_test test = {
>  #ifdef __x86_64__

> 	.run = run,
>  #endif
> 	.supported_archs = {"x86_64", NULL},
>  }

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: do_setup(): Check for supported arch first

[Cyril Hrubis]

Hi!
> > This commit moves the check for supported architecture before the check
> > for test function existence. This allows us do ifdef out the run
> > function pointer initialization and properly TCONF on unsupported
> > platform.
> 
> LGTM. Ideally could you also convert some test (as example which others
> can follow - like they do for docparse fixes).
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> > Example usage:
> 
> >  #include "tst_test.h"
> 
> >  #ifdef __x86_64__
> I would personally did not bother with #ifdef, but it's true it makes resulted
> binary smaller (for non-intel arch).

It's not about tinification, it's usually that there is inline assembly
that cannot be compiled at all...

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp