[v2,3/3] testcase: get rid of compiling errors
diff mbox series

Message ID 20190615042048.29839-3-liwang@redhat.com
State New
Headers show
Series
  • [v2,1/3] lib: adding .arch field in tst_test structure
Related show

Commit Message

Li Wang June 15, 2019, 4:20 a.m. UTC
Signed-off-by: Li Wang <liwang@redhat.com>
---
 configure.ac             | 1 +
 testcases/cve/meltdown.c | 5 +++++
 2 files changed, 6 insertions(+)

Comments

Petr Vorel June 17, 2019, 9:42 p.m. UTC | #1
Hi Li,

> Signed-off-by: Li Wang <liwang@redhat.com>

Thanks this patchset.

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

> ---
>  configure.ac             | 1 +
>  testcases/cve/meltdown.c | 5 +++++
>  2 files changed, 6 insertions(+)

> diff --git a/configure.ac b/configure.ac
> index 5ecc92781..521f56541 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -58,6 +58,7 @@ AC_CHECK_HEADERS([ \
>      sys/shm.h \
>      sys/ustat.h \
>      sys/xattr.h \
> +    emmintrin.h \
Just AC_CHECK_HEADERS is sorted alphabetically since a50338cac.
But this can be amended during merging.


Kind regards,
Petr

>  ])

>  AC_CHECK_FUNCS([ \
> diff --git a/testcases/cve/meltdown.c b/testcases/cve/meltdown.c
> index 72c9ec907..bc649b893 100644
> --- a/testcases/cve/meltdown.c
> +++ b/testcases/cve/meltdown.c
> @@ -29,6 +29,7 @@
>  #include <ctype.h>
>  #include <sys/utsname.h>

> +#ifdef HAVE_EMMINTRIN_H
>  #include <emmintrin.h>

>  #include "libtsc.h"
> @@ -387,3 +388,7 @@ static struct tst_test test = {
>  	.cleanup = cleanup,
>  	.min_kver = "2.6.32"
>  };
> +
> +#else /* HAVE_EMMINTRIN_H */
> +	TST_TEST_TCONF("<emmintrin.h> is not supported");
> +#endif
Jan Stancek June 17, 2019, 9:44 p.m. UTC | #2
----- Original Message -----
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  configure.ac             | 1 +
>  testcases/cve/meltdown.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 5ecc92781..521f56541 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -58,6 +58,7 @@ AC_CHECK_HEADERS([ \
>      sys/shm.h \
>      sys/ustat.h \
>      sys/xattr.h \
> +    emmintrin.h \
>  ])
>  
>  AC_CHECK_FUNCS([ \
> diff --git a/testcases/cve/meltdown.c b/testcases/cve/meltdown.c
> index 72c9ec907..bc649b893 100644
> --- a/testcases/cve/meltdown.c
> +++ b/testcases/cve/meltdown.c
> @@ -29,6 +29,7 @@
>  #include <ctype.h>
>  #include <sys/utsname.h>
>  
> +#ifdef HAVE_EMMINTRIN_H
>  #include <emmintrin.h>
>  
>  #include "libtsc.h"
> @@ -387,3 +388,7 @@ static struct tst_test test = {
>  	.cleanup = cleanup,
>  	.min_kver = "2.6.32"
>  };
> +
> +#else /* HAVE_EMMINTRIN_H */
> +	TST_TEST_TCONF("<emmintrin.h> is not supported");
> +#endif

This seems more complicated to me than original - extra autoconf check, extra ifdef.

I can see how tst_on_arch() would be useful. Test is valid on all arches,
but needs different input/constants/code/etc.

What is the motivation for tst_test.arch? Is it to have some annotation in tst_test struct?

> --
> 2.20.1
> 
> 
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
Li Wang June 18, 2019, 4:03 a.m. UTC | #3
On Tue, Jun 18, 2019 at 5:44 AM Jan Stancek <jstancek@redhat.com> wrote:

>
> ----- Original Message -----
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > ---
> >  configure.ac             | 1 +
> >  testcases/cve/meltdown.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 5ecc92781..521f56541 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -58,6 +58,7 @@ AC_CHECK_HEADERS([ \
> >      sys/shm.h \
> >      sys/ustat.h \
> >      sys/xattr.h \
> > +    emmintrin.h \
> >  ])
> >
> >  AC_CHECK_FUNCS([ \
> > diff --git a/testcases/cve/meltdown.c b/testcases/cve/meltdown.c
> > index 72c9ec907..bc649b893 100644
> > --- a/testcases/cve/meltdown.c
> > +++ b/testcases/cve/meltdown.c
> > @@ -29,6 +29,7 @@
> >  #include <ctype.h>
> >  #include <sys/utsname.h>
> >
> > +#ifdef HAVE_EMMINTRIN_H
> >  #include <emmintrin.h>
> >
> >  #include "libtsc.h"
> > @@ -387,3 +388,7 @@ static struct tst_test test = {
> >       .cleanup = cleanup,
> >       .min_kver = "2.6.32"
> >  };
> > +
> > +#else /* HAVE_EMMINTRIN_H */
> > +     TST_TEST_TCONF("<emmintrin.h> is not supported");
> > +#endif
>
> This seems more complicated to me than original - extra autoconf check,
> extra ifdef.
>

Indeed.
I choose meltdown.c as a demo randomly, this patch is to solve the
compiling error after removing ifdef __arch__ from code. So, as you can see
the tst_test.arch is not perfect, but it could be used for some general
situations.


> I can see how tst_on_arch() would be useful. Test is valid on all arches,
> but needs different input/constants/code/etc.
>

That's the original reason I exported it in patch v1.

Cyril and I have discussed this part, we think tst_on_arch() is not cleaner
than ifdefs, so it's not suggested to use it directly in a test case.


>
> What is the motivation for tst_test.arch? Is it to have some annotation in
> tst_test struct?
>

The first motivation is to save LTP user from arch ifdefs, moves the
information from code to tst_test metadata, but seems that it can not be
completely replaced because of the specific assembly inline. So I still
hope to find a better way for that, even not go with this tst_test.arch.

Patch
diff mbox series

diff --git a/configure.ac b/configure.ac
index 5ecc92781..521f56541 100644
--- a/configure.ac
+++ b/configure.ac
@@ -58,6 +58,7 @@  AC_CHECK_HEADERS([ \
     sys/shm.h \
     sys/ustat.h \
     sys/xattr.h \
+    emmintrin.h \
 ])
 
 AC_CHECK_FUNCS([ \
diff --git a/testcases/cve/meltdown.c b/testcases/cve/meltdown.c
index 72c9ec907..bc649b893 100644
--- a/testcases/cve/meltdown.c
+++ b/testcases/cve/meltdown.c
@@ -29,6 +29,7 @@ 
 #include <ctype.h>
 #include <sys/utsname.h>
 
+#ifdef HAVE_EMMINTRIN_H
 #include <emmintrin.h>
 
 #include "libtsc.h"
@@ -387,3 +388,7 @@  static struct tst_test test = {
 	.cleanup = cleanup,
 	.min_kver = "2.6.32"
 };
+
+#else /* HAVE_EMMINTRIN_H */
+	TST_TEST_TCONF("<emmintrin.h> is not supported");
+#endif