Re: [PATCH 3/3] kunit: test: Drop CONFIG_MODULE ifdeffery

[Petr Pavlu <petr.pavlu@suse.com>]

On 6/17/25 10:39 AM, Thomas Weißschuh wrote:
> On Tue, Jun 17, 2025 at 09:44:49AM +0200, Petr Pavlu wrote:
>> On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
>>> The function stubs exposed by module.h allow the code to compile properly
>>> without the ifdeffery. The generated object code stays the same, as the
>>> compiler can optimize away all the dead code.
>>> As the code is still typechecked developer errors can be detected faster.
>>>
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>
>> I'm worried that patches #2 and #3 make the code harder to understand
>> because they hide what is compiled and when.
>>
>> Normally, '#ifdef CONFIG_XYZ' or IS_ENABLED(CONFIG_XYZ) directly
>> surrounds functionality that should be conditional. This makes it clear
>> what is used and when.
> 
> #ifdef is discouraged in C files and IS_ENABLED(CONFIG_MODULES) does not work
> (here) without patch #2.
> 
>> The patches obscure whether, for instance, kunit_module_notify() in
>> lib/kunit/test.c is actually used and present in the resulting binary
>> behind several steps. Understanding its usage requires tracing the code
>> from kunit_module_notify() to the definition of kunit_mod_nb, then to
>> the register_module_notifier() call, and finally depends on an ifdef in
>> another file, include/linux/module.h.
> 
> I agree that it is not completely clear what will end up in the binary.
> On the other hand we can program the happy path and the compiler will take care
> of all the corner cases.
> We could add an "if (IS_ENABLED(CONFIG_MODULES))" which does not really change
> anything but would be clearer to read.
> 
>> Is this really better? Are there places where this pattern is already
>> used? Does it actually help in practice, considering that CONFIG_MODULES
>> is enabled in most cases?
> 
> This came up for me when refactoring some KUnit internal code.
> I used "kunit.py run" (which uses CONFIG_MODULES=n) to test my changes.
> But some callers of changed functions were not updated and this wasn't reported.

I see.

> 
> The stub functions are a standard pattern and already implemented by module.h.

Stub functions are ok, I have no concerns about that pattern.

> I have not found a header which hides structure definitions.

It seems you're right. I think that makes patch #2 acceptable, it is
consistent with other kernel code.

> 
> Documentation/process/coding-style.rst:
> 
> 	21) Conditional Compilation
> 	---------------------------
> 
> 	Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> 	files; doing so makes code harder to read and logic harder to follow.  Instead,
> 	use such conditionals in a header file defining functions for use in those .c
> 	files, providing no-op stub versions in the #else case, and then call those
> 	functions unconditionally from .c files.  The compiler will avoid generating
> 	any code for the stub calls, producing identical results, but the logic will
> 	remain easy to follow.
> 
> I should add the documentation reference to patch #2.

This part discusses stub functions. I feel patch #3 stretches the
intention of the coding style described here. As discussed, the patch
somewhat hides when the functions are actually used, which might not be
desirable, but I'll leave it to the kunit folks to decide what they
prefer.

-- 
Thanks,
Petr