Re: [LTP] [PATCH 1/2] ptrace07: Fix compilation when not on x86

[Richard Palethorpe <rpalethorpe@suse.de>]

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