All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86
@ 2022-10-18 15:25 Richard Palethorpe via ltp
  2022-10-18 15:25 ` [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc Richard Palethorpe via ltp
  2022-10-19  9:30 ` [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Martin Doucha
  0 siblings, 2 replies; 11+ messages in thread
From: Richard Palethorpe via ltp @ 2022-10-18 15:25 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

Strictly cpuid.h should not be included on non-x86, but this means
each test author has to remember not to include it. Instead we can set
a blank macro to allow compilation.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

This time I created a PR to check if compilation succeeds:
https://github.com/linux-test-project/ltp/pull/984

 include/lapi/cpuid.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/lapi/cpuid.h b/include/lapi/cpuid.h
index cd0567f92..20e977093 100644
--- a/include/lapi/cpuid.h
+++ b/include/lapi/cpuid.h
@@ -16,11 +16,15 @@
  * have __cpuid_count().
  */
 #ifndef __cpuid_count
+#  if defined(__i386__) || defined(__x86_64__)
 #define __cpuid_count(level, count, a, b, c, d) ({			\
 	__asm__ __volatile__ ("cpuid\n\t"				\
 			      : "=a" (a), "=b" (b), "=c" (c), "=d" (d)	\
 			      : "0" (level), "2" (count));		\
 })
+#  else
+#define __cpuid_count(level, count, a, b, c, d)
+#  endif
 #endif
 
 #endif /* LAPI_CPUID_H__ */
-- 
2.36.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc
  2022-10-18 15:25 [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Richard Palethorpe via ltp
@ 2022-10-18 15:25 ` Richard Palethorpe via ltp
  2022-10-19  1:40   ` Pengfei Xu
  2022-10-19  9:30 ` [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Martin Doucha
  1 sibling, 1 reply; 11+ messages in thread
From: Richard Palethorpe via ltp @ 2022-10-18 15:25 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

Use memalign instead because we live in the past.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Cc: Pengfei Xu <pengfei.xu@intel.com>
---
 testcases/kernel/syscalls/ptrace/ptrace07.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c b/testcases/kernel/syscalls/ptrace/ptrace07.c
index 6bd2b1062..a60c2a49e 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace07.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
@@ -35,6 +35,7 @@
 #include "config.h"
 #include "ptrace.h"
 #include "tst_test.h"
+#include "tst_safe_macros.h"
 #include "lapi/cpuid.h"
 
 #ifndef PTRACE_GETREGSET
@@ -95,7 +96,7 @@ static void do_test(void)
 	 * of the XSAVE/XRSTOR save area) required by enabled features in XCR0.
 	 */
 	__cpuid_count(CPUID_LEAF_XSTATE, ecx, eax, ebx, ecx, edx);
-	xstate = aligned_alloc(64, ebx);
+	xstate = SAFE_MEMALIGN(64, ebx);
 	struct iovec iov = { .iov_base = xstate, .iov_len = ebx };
 	int status;
 	bool okay;
-- 
2.36.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc
  2022-10-18 15:25 ` [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc Richard Palethorpe via ltp
@ 2022-10-19  1:40   ` Pengfei Xu
  2022-10-19  2:59     ` Li Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Pengfei Xu @ 2022-10-19  1:40 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: ltp

Hi Richard,

On 2022-10-18 at 16:25:27 +0100, Richard Palethorpe wrote:
> Use memalign instead because we live in the past.
> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Cc: Pengfei Xu <pengfei.xu@intel.com>
> ---
>  testcases/kernel/syscalls/ptrace/ptrace07.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c b/testcases/kernel/syscalls/ptrace/ptrace07.c
> index 6bd2b1062..a60c2a49e 100644
> --- a/testcases/kernel/syscalls/ptrace/ptrace07.c
> +++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
> @@ -35,6 +35,7 @@
>  #include "config.h"
>  #include "ptrace.h"
>  #include "tst_test.h"
> +#include "tst_safe_macros.h"
>  #include "lapi/cpuid.h"
>  
>  #ifndef PTRACE_GETREGSET
> @@ -95,7 +96,7 @@ static void do_test(void)
>  	 * of the XSAVE/XRSTOR save area) required by enabled features in XCR0.
>  	 */
>  	__cpuid_count(CPUID_LEAF_XSTATE, ecx, eax, ebx, ecx, edx);
> -	xstate = aligned_alloc(64, ebx);
> +	xstate = SAFE_MEMALIGN(64, ebx);
>  	struct iovec iov = { .iov_base = xstate, .iov_len = ebx };
>  	int status;
>  	bool okay;

Yes, it's better for LTP compilation. Thanks for the patch!

I checked SAFE_MEMALIGN(), it will verify that the buffer is NULL or not.
"
	rval = memalign(alignment, size);

	if (rval == NULL) {
		tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
			"memalign() failed");
	}
"
So could you remove below unnecessary lines in ptrace07.c, thanks.
"
-	if (!xstate)
-		tst_brk(TBROK, "aligned_alloc() failed for xstate buffer");
"

Thanks!
BR.

> -- 
> 2.36.1
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc
  2022-10-19  1:40   ` Pengfei Xu
@ 2022-10-19  2:59     ` Li Wang
  2022-10-19  3:16       ` Pengfei Xu
  0 siblings, 1 reply; 11+ messages in thread
From: Li Wang @ 2022-10-19  2:59 UTC (permalink / raw)
  To: Pengfei Xu; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 1791 bytes --]

On Wed, Oct 19, 2022 at 9:40 AM Pengfei Xu <pengfei.xu@intel.com> wrote:

> Hi Richard,
>
> On 2022-10-18 at 16:25:27 +0100, Richard Palethorpe wrote:
> > Use memalign instead because we live in the past.
> >
> > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > Cc: Pengfei Xu <pengfei.xu@intel.com>
> > ---
> >  testcases/kernel/syscalls/ptrace/ptrace07.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c
> b/testcases/kernel/syscalls/ptrace/ptrace07.c
> > index 6bd2b1062..a60c2a49e 100644
> > --- a/testcases/kernel/syscalls/ptrace/ptrace07.c
> > +++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
> > @@ -35,6 +35,7 @@
> >  #include "config.h"
> >  #include "ptrace.h"
> >  #include "tst_test.h"
> > +#include "tst_safe_macros.h"
> >  #include "lapi/cpuid.h"
> >
> >  #ifndef PTRACE_GETREGSET
> > @@ -95,7 +96,7 @@ static void do_test(void)
> >        * of the XSAVE/XRSTOR save area) required by enabled features in
> XCR0.
> >        */
> >       __cpuid_count(CPUID_LEAF_XSTATE, ecx, eax, ebx, ecx, edx);
> > -     xstate = aligned_alloc(64, ebx);
> > +     xstate = SAFE_MEMALIGN(64, ebx);
> >       struct iovec iov = { .iov_base = xstate, .iov_len = ebx };
> >       int status;
> >       bool okay;
>
> Yes, it's better for LTP compilation. Thanks for the patch!
>
> I checked SAFE_MEMALIGN(), it will verify that the buffer is NULL or not.
> "
>         rval = memalign(alignment, size);
>
>         if (rval == NULL) {
>                 tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
>                         "memalign() failed");
>         }
> "
> So could you remove below unnecessary lines in ptrace07.c, thanks.
>

I helped modify this and pushed it, thanks!

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2830 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc
  2022-10-19  2:59     ` Li Wang
@ 2022-10-19  3:16       ` Pengfei Xu
  2022-10-19  3:20         ` Li Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Pengfei Xu @ 2022-10-19  3:16 UTC (permalink / raw)
  To: Li Wang; +Cc: Richard Palethorpe, ltp

Hi Li Wang,
On 2022-10-19 at 10:59:54 +0800, Li Wang wrote:
> On Wed, Oct 19, 2022 at 9:40 AM Pengfei Xu <pengfei.xu@intel.com> wrote:
> 
> > Hi Richard,
> >
> > On 2022-10-18 at 16:25:27 +0100, Richard Palethorpe wrote:
> > > Use memalign instead because we live in the past.
> > >
> > > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > > Cc: Pengfei Xu <pengfei.xu@intel.com>
> > > ---
> > >  testcases/kernel/syscalls/ptrace/ptrace07.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c
> > b/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > index 6bd2b1062..a60c2a49e 100644
> > > --- a/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > +++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > @@ -35,6 +35,7 @@
> > >  #include "config.h"
> > >  #include "ptrace.h"
> > >  #include "tst_test.h"
> > > +#include "tst_safe_macros.h"
> > >  #include "lapi/cpuid.h"
> > >
> > >  #ifndef PTRACE_GETREGSET
> > > @@ -95,7 +96,7 @@ static void do_test(void)
> > >        * of the XSAVE/XRSTOR save area) required by enabled features in
> > XCR0.
> > >        */
> > >       __cpuid_count(CPUID_LEAF_XSTATE, ecx, eax, ebx, ecx, edx);
> > > -     xstate = aligned_alloc(64, ebx);
> > > +     xstate = SAFE_MEMALIGN(64, ebx);
> > >       struct iovec iov = { .iov_base = xstate, .iov_len = ebx };
> > >       int status;
> > >       bool okay;
> >
> > Yes, it's better for LTP compilation. Thanks for the patch!
> >
> > I checked SAFE_MEMALIGN(), it will verify that the buffer is NULL or not.
> > "
> >         rval = memalign(alignment, size);
> >
> >         if (rval == NULL) {
> >                 tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
> >                         "memalign() failed");
> >         }
> > "
> > So could you remove below unnecessary lines in ptrace07.c, thanks.
> >
> 
> I helped modify this and pushed it, thanks!
  Great, thanks!
  I tried that "SAFE_MEMALIGN(64, ebx);" modification in ptrace07.
  Ptrace07 works well and passed in previous reproduced server.

  Thanks!
  BR.
> 
> -- 
> Regards,
> Li Wang

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc
  2022-10-19  3:16       ` Pengfei Xu
@ 2022-10-19  3:20         ` Li Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Li Wang @ 2022-10-19  3:20 UTC (permalink / raw)
  To: Pengfei Xu; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 2329 bytes --]

On Wed, Oct 19, 2022 at 11:17 AM Pengfei Xu <pengfei.xu@intel.com> wrote:

> Hi Li Wang,
> On 2022-10-19 at 10:59:54 +0800, Li Wang wrote:
> > On Wed, Oct 19, 2022 at 9:40 AM Pengfei Xu <pengfei.xu@intel.com> wrote:
> >
> > > Hi Richard,
> > >
> > > On 2022-10-18 at 16:25:27 +0100, Richard Palethorpe wrote:
> > > > Use memalign instead because we live in the past.
> > > >
> > > > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > > > Cc: Pengfei Xu <pengfei.xu@intel.com>
> > > > ---
> > > >  testcases/kernel/syscalls/ptrace/ptrace07.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > b/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > > index 6bd2b1062..a60c2a49e 100644
> > > > --- a/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > > +++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
> > > > @@ -35,6 +35,7 @@
> > > >  #include "config.h"
> > > >  #include "ptrace.h"
> > > >  #include "tst_test.h"
> > > > +#include "tst_safe_macros.h"
> > > >  #include "lapi/cpuid.h"
> > > >
> > > >  #ifndef PTRACE_GETREGSET
> > > > @@ -95,7 +96,7 @@ static void do_test(void)
> > > >        * of the XSAVE/XRSTOR save area) required by enabled features
> in
> > > XCR0.
> > > >        */
> > > >       __cpuid_count(CPUID_LEAF_XSTATE, ecx, eax, ebx, ecx, edx);
> > > > -     xstate = aligned_alloc(64, ebx);
> > > > +     xstate = SAFE_MEMALIGN(64, ebx);
> > > >       struct iovec iov = { .iov_base = xstate, .iov_len = ebx };
> > > >       int status;
> > > >       bool okay;
> > >
> > > Yes, it's better for LTP compilation. Thanks for the patch!
> > >
> > > I checked SAFE_MEMALIGN(), it will verify that the buffer is NULL or
> not.
> > > "
> > >         rval = memalign(alignment, size);
> > >
> > >         if (rval == NULL) {
> > >                 tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
> > >                         "memalign() failed");
> > >         }
> > > "
> > > So could you remove below unnecessary lines in ptrace07.c, thanks.
> > >
> >
> > I helped modify this and pushed it, thanks!
>   Great, thanks!
>   I tried that "SAFE_MEMALIGN(64, ebx);" modification in ptrace07.
>   Ptrace07 works well and passed in previous reproduced server.
>

Cool~ thanks for your feedback.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 3836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86
  2022-10-18 15:25 [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Richard Palethorpe via ltp
  2022-10-18 15:25 ` [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc Richard Palethorpe via ltp
@ 2022-10-19  9:30 ` Martin Doucha
  2022-10-20  3:53   ` Li Wang
  1 sibling, 1 reply; 11+ messages in thread
From: Martin Doucha @ 2022-10-19  9:30 UTC (permalink / raw)
  To: Richard Palethorpe, ltp

On 18. 10. 22 17:25, Richard Palethorpe via ltp wrote:
> Strictly cpuid.h should not be included on non-x86, but this means
> each test author has to remember not to include it. Instead we can set
> a blank macro to allow compilation.
> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---

Hi,
defining an empty macro is a bad idea because it'll allow a broken test 
to be compiled and then it'll behave erratically for no apparent reason. 
It's better to get a build failure.

Reverting 1df4de06206b079f24dde7157d037b48727c743d is the best solution 
here. Building ptrace07 and similar arch-specific tests without a key 
piece of code does not make sense. The preprocessor arch checks should 
wrap around the whole file, not just a small non-portable bit that's 
crucial for the test to work.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86
  2022-10-19  9:30 ` [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Martin Doucha
@ 2022-10-20  3:53   ` Li Wang
  2022-10-20  7:31     ` Richard Palethorpe
  2022-10-20 10:41     ` Martin Doucha
  0 siblings, 2 replies; 11+ messages in thread
From: Li Wang @ 2022-10-20  3:53 UTC (permalink / raw)
  To: Martin Doucha; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 1438 bytes --]

On Wed, Oct 19, 2022 at 5:30 PM Martin Doucha <mdoucha@suse.cz> wrote:

> On 18. 10. 22 17:25, Richard Palethorpe via ltp wrote:
> > Strictly cpuid.h should not be included on non-x86, but this means
> > each test author has to remember not to include it. Instead we can set
> > a blank macro to allow compilation.
> >
> > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > ---
>
> Hi,
> defining an empty macro is a bad idea because it'll allow a broken test
> to be compiled and then it'll behave erratically for no apparent reason.
> It's better to get a build failure.
>
> Reverting 1df4de06206b079f24dde7157d037b48727c743d is the best solution
> here. Building ptrace07 and similar arch-specific tests without a key
> piece of code does not make sense. The preprocessor arch checks should
> wrap around the whole file, not just a small non-portable bit that's
> crucial for the test to work.
>

From what I know, one of the uses of "empty macro" is to conditionally
include certain portions of a program. In ptrace07, it invokes that useless
macro for compiling pass on non-x86 arch but does not allow execute it.

I don't see why that's crucial for a test, if we wrap around the whole file
and
avoid it compiling on non-x86, isn't this essentially same as that?

The only distinction between them is partly or wholly skipping the key
code compilation. or maybe I completely misunderstood this part.


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2417 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86
  2022-10-20  3:53   ` Li Wang
@ 2022-10-20  7:31     ` Richard Palethorpe
  2022-10-20 10:41     ` Martin Doucha
  1 sibling, 0 replies; 11+ messages in thread
From: Richard Palethorpe @ 2022-10-20  7:31 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hello Li,

Li Wang <liwang@redhat.com> writes:

> On Wed, Oct 19, 2022 at 5:30 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
>  On 18. 10. 22 17:25, Richard Palethorpe via ltp wrote:
>  > Strictly cpuid.h should not be included on non-x86, but this means
>  > each test author has to remember not to include it. Instead we can set
>  > a blank macro to allow compilation.
>  > 
>  > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>  > ---
>
>  Hi,
>  defining an empty macro is a bad idea because it'll allow a broken test 
>  to be compiled and then it'll behave erratically for no apparent reason. 
>  It's better to get a build failure.
>
>  Reverting 1df4de06206b079f24dde7157d037b48727c743d is the best solution 
>  here. Building ptrace07 and similar arch-specific tests without a key 
>  piece of code does not make sense. The preprocessor arch checks should 
>  wrap around the whole file, not just a small non-portable bit that's 
>  crucial for the test to work.
>
> From what I know, one of the uses of "empty macro" is to conditionally
> include certain portions of a program. In ptrace07, it invokes that useless
> macro for compiling pass on non-x86 arch but does not allow execute it.
>
> I don't see why that's crucial for a test, if we wrap around the whole file and
> avoid it compiling on non-x86, isn't this essentially same as that? 
>
> The only distinction between them is partly or wholly skipping the key
> code compilation. or maybe I completely misunderstood this part.

Yes that's it. Since we decided to skip arches depending on what is
tst_test instead of compiling out the whole test. Which we want to do
because it provides meta-data.

I guess we could define the macro as ts_brk(TCONF, ...). However another
problem I just thought of is that cpuid.h might exist on a non-x86
system. So we'll still get a compilation failure.

Ideally we want compilation to fail even on x86 if the correct guards
are not in place and we include cpuid.h.

So we could use some macros like TST_ARCH_GUARD_X86 which is defined by
TST_IF_X86 and undefined by TST_ELSE_X86/TST_ENDIF_X86. Then cpuid.h can
check for that macro.

We could also define assembly in TST_ASM_X86 etc., although it's more
obvious that won't compile on non x86.

Alternatively I could try doing something in Sparse, but the macro pass
is maybe not as easy to hook into.

-- 
Thank you,
Richard.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86
  2022-10-20  3:53   ` Li Wang
  2022-10-20  7:31     ` Richard Palethorpe
@ 2022-10-20 10:41     ` Martin Doucha
  2022-10-20 10:59       ` Richard Palethorpe
  1 sibling, 1 reply; 11+ messages in thread
From: Martin Doucha @ 2022-10-20 10:41 UTC (permalink / raw)
  To: Li Wang; +Cc: Richard Palethorpe, ltp

On 20. 10. 22 5:53, Li Wang wrote:
> 
> 
> On Wed, Oct 19, 2022 at 5:30 PM Martin Doucha <mdoucha@suse.cz 
> <mailto:mdoucha@suse.cz>> wrote:
>     Reverting 1df4de06206b079f24dde7157d037b48727c743d is the best solution
>     here. Building ptrace07 and similar arch-specific tests without a key
>     piece of code does not make sense. The preprocessor arch checks should
>     wrap around the whole file, not just a small non-portable bit that's
>     crucial for the test to work.
> 
>  From what I know, one of the uses of "empty macro" is to conditionally
> include certain portions of a program. In ptrace07, it invokes that useless
> macro for compiling pass on non-x86 arch but does not allow execute it.
> 
> I don't see why that's crucial for a test, if we wrap around the whole 
> file and
> avoid it compiling on non-x86, isn't this essentially same as that?
> 
> The only distinction between them is partly or wholly skipping the key
> code compilation. or maybe I completely misunderstood this part.

The code that would be generated by the non-empty version of the macro 
is crucial for rest of the program to work. Putting conditional build 
directives only around a few lines of code can cause bogus warnings 
about uninitialized variables and make it difficult to add more 
arch-specific code like the cpuid_count() macro. There's nothing wrong 
with writing tests like this:

#include "tst_test.h"

#ifdef __x86_64__
#include "lapi/cpuid.h"

void setup(void)
{
	...
}

void run(void)
{
	...
}

void cleanup(void)
{
	...
}

static struct tst_test test = {
	...
	.supported_archs = (const char *const []) {
		"x86_64",
		NULL
	},
};

#else /* __x86_64__ */
#define TST_TEST_TCONF("this test is only supported on x86_64")
#endif /* __x86_64__ */


IIUC, the metadata parser will still read .supported_archs regardless of 
conditional build directives. And it'll prevent erratic test behavior in 
edge cases where the LTP library believes the code was compiled for the 
right architecture but the GCC preprocessor disagrees.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86
  2022-10-20 10:41     ` Martin Doucha
@ 2022-10-20 10:59       ` Richard Palethorpe
  0 siblings, 0 replies; 11+ messages in thread
From: Richard Palethorpe @ 2022-10-20 10:59 UTC (permalink / raw)
  To: Martin Doucha; +Cc: ltp

Hello,

Martin Doucha <mdoucha@suse.cz> writes:

> On 20. 10. 22 5:53, Li Wang wrote:
>> On Wed, Oct 19, 2022 at 5:30 PM Martin Doucha <mdoucha@suse.cz
>> <mailto:mdoucha@suse.cz>> wrote:
>>     Reverting 1df4de06206b079f24dde7157d037b48727c743d is the best solution
>>     here. Building ptrace07 and similar arch-specific tests without a key
>>     piece of code does not make sense. The preprocessor arch checks should
>>     wrap around the whole file, not just a small non-portable bit that's
>>     crucial for the test to work.
>>  From what I know, one of the uses of "empty macro" is to
>> conditionally
>> include certain portions of a program. In ptrace07, it invokes that useless
>> macro for compiling pass on non-x86 arch but does not allow execute it.
>> I don't see why that's crucial for a test, if we wrap around the
>> whole file and
>> avoid it compiling on non-x86, isn't this essentially same as that?
>> The only distinction between them is partly or wholly skipping the
>> key
>> code compilation. or maybe I completely misunderstood this part.
>
> The code that would be generated by the non-empty version of the macro
> is crucial for rest of the program to work. Putting conditional build
> directives only around a few lines of code can cause bogus warnings
> about uninitialized variables and make it difficult to add more
> arch-specific code like the cpuid_count() macro. There's nothing wrong
> with writing tests like this:
>
> #include "tst_test.h"
>
> #ifdef __x86_64__
> #include "lapi/cpuid.h"
>
> void setup(void)
> {
> 	...
> }
>
> void run(void)
> {
> 	...
> }
>
> void cleanup(void)
> {
> 	...
> }
>
> static struct tst_test test = {
> 	...
> 	.supported_archs = (const char *const []) {
> 		"x86_64",
> 		NULL
> 	},
> };
>
> #else /* __x86_64__ */
> #define TST_TEST_TCONF("this test is only supported on x86_64")
> #endif /* __x86_64__ */
>
>
> IIUC, the metadata parser will still read .supported_archs regardless
> of conditional build directives. And it'll prevent erratic test
> behavior in edge cases where the LTP library believes the code was
> compiled for the right architecture but the GCC preprocessor
> disagrees.

Yes, this sounds reasonable and I will submit a patch for it (on Monday
though).

My remaining concern is that people will include lapi/cpuid.h (or
similar) outside of the ifdef and it will not be caught until it's
merged into master.

-- 
Thank you,
Richard.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-10-20 11:09 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-18 15:25 [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Richard Palethorpe via ltp
2022-10-18 15:25 ` [LTP] [PATCH 2/2] ptrace07: Fix compilation by avoiding aligned_alloc Richard Palethorpe via ltp
2022-10-19  1:40   ` Pengfei Xu
2022-10-19  2:59     ` Li Wang
2022-10-19  3:16       ` Pengfei Xu
2022-10-19  3:20         ` Li Wang
2022-10-19  9:30 ` [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86 Martin Doucha
2022-10-20  3:53   ` Li Wang
2022-10-20  7:31     ` Richard Palethorpe
2022-10-20 10:41     ` Martin Doucha
2022-10-20 10:59       ` Richard Palethorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.