[PATCH] kunit: Fix 'hooks.o' build by recursing into kunit

[PATCH] kunit: Fix 'hooks.o' build by recursing into kunit

[David Gow]

KUnit's 'hooks.o' file need to be built-in whenever KUnit is enabled
(even if CONFIG_KUNIT=m). We'd previously attemtped to do this by adding
'kunit/hooks.o' to obj-y in lib/Makefile, but this caused hooks.c to be
rebuilt even when it was unchanged.

Instead, always recurse into lib/kunit using obj-y when KUnit is
enabled, and add the hooks there.

Fixes: 7170b7ed6acb ("kunit: Add "hooks" to call into KUnit when it's built as a module").
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/linux-kselftest/CAHk-=wiEf7irTKwPJ0jTMOF3CS-13UXmF6Fns3wuWpOZ_wGyZQ@mail.gmail.com/
Signed-off-by: David Gow <davidgow@google.com>
---

I like this way of handling the makefiles much better. I had tried it
when originally writing the hooks patch and not managed to get it
working. Not sure what's changed now, but it works in all of the usual
cases (CONFIG_KUNIT={n,y,m}, kunit.py run, etc).

Linus, Shuah: Let me know if you want this to go via the KUnit branch,
or if you just want to apply it directly and get rid of the annoyances
ASAP.

---
 lib/Makefile       | 12 ++++--------
 lib/kunit/Makefile |  2 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index 469be6240523..baf2821f7a00 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -127,14 +127,10 @@ CFLAGS_test_fpu.o += $(FPU_CFLAGS)
 
 obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
 
-obj-$(CONFIG_KUNIT) += kunit/
-# Include the KUnit hooks unconditionally. They'll compile to nothing if
-# CONFIG_KUNIT=n, otherwise will be a small table of static data (static key,
-# function pointers) which need to be built-in even when KUnit is a module.
-ifeq ($(CONFIG_KUNIT), m)
-obj-y += kunit/hooks.o
-else
-obj-$(CONFIG_KUNIT) += kunit/hooks.o
+# Some KUnit files (hooks.o) need to be built-in even when KUnit is a module,
+# so we can't just use obj-$(CONFIG_KUNIT).
+ifdef CONFIG_KUNIT
+obj-y += kunit/
 endif
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index da665cd4ea12..cb417f504996 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -13,7 +13,7 @@ kunit-objs +=				debugfs.o
 endif
 
 # KUnit 'hooks' are built-in even when KUnit is built as a module.
-lib-y +=				hooks.o
+obj-y +=				hooks.o
 
 obj-$(CONFIG_KUNIT_TEST) +=		kunit-test.o
 
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH] kunit: Fix 'hooks.o' build by recursing into kunit

[Brendan Higgins]

On Fri, Feb 24, 2023 at 8:45 PM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> KUnit's 'hooks.o' file need to be built-in whenever KUnit is enabled
> (even if CONFIG_KUNIT=m). We'd previously attemtped to do this by adding
> 'kunit/hooks.o' to obj-y in lib/Makefile, but this caused hooks.c to be
> rebuilt even when it was unchanged.
>
> Instead, always recurse into lib/kunit using obj-y when KUnit is
> enabled, and add the hooks there.
>
> Fixes: 7170b7ed6acb ("kunit: Add "hooks" to call into KUnit when it's built as a module").
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/linux-kselftest/CAHk-=wiEf7irTKwPJ0jTMOF3CS-13UXmF6Fns3wuWpOZ_wGyZQ@mail.gmail.com/
> Signed-off-by: David Gow <davidgow@google.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] kunit: Fix 'hooks.o' build by recursing into kunit

[Linus Torvalds]

On Fri, Feb 24, 2023 at 5:45 PM David Gow <davidgow@google.com> wrote:
>
> +# Some KUnit files (hooks.o) need to be built-in even when KUnit is a module,
> +# so we can't just use obj-$(CONFIG_KUNIT).
> +ifdef CONFIG_KUNIT
> +obj-y += kunit/
>  endif

We actually have a pattern for this, although I guess it's rare enough
that "pattern" isn't necessarily the right word.

But you can find things like the Hyper-V drivers having similar
issues, and so the driver Makefile has

    obj-$(subst m,y,$(CONFIG_HYPERV))       += hv/

See a few other cases with

    git grep "subst m,y,"

but I guess the "ifdef CONFIG_KUNIT" thing works too. I can only find
one case of that (in arch/mips/Kbuild).

Another way of dealing with this - that is more common for individual
object files rather than directories - is to just do

    kunit-dir-$(CONFIG_KUNIT) := kunit/
    obj-y += $(kunit-dir-y) $(kunit-dir-m)

which admittedly is also not a hugely common pattern, but does exist
in various places (see for example the 'sfp-bus.o' file and CONFIG_SFP
in drivers/net/phy/Makefile.

That last pattern is probably most common in scripts/Makefile.lib,
where we have things like

       hostprogs += $(hostprogs-always-y) $(hostprogs-always-m)

which is similar but not the exact same thing.

Anyway, I guess I'll just apply that patch as-is, I just wanted to
point out that the particular pattern it uses may be simple, but we've
generally tried to just do our Makefile evaluations with "arithmetic"
rather than conditionals.

              Linus

Re: [PATCH] kunit: Fix 'hooks.o' build by recursing into kunit

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

On Tue, 28 Feb 2023 at 06:43, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Feb 24, 2023 at 5:45 PM David Gow <davidgow@google.com> wrote:
> >
> > +# Some KUnit files (hooks.o) need to be built-in even when KUnit is a module,
> > +# so we can't just use obj-$(CONFIG_KUNIT).
> > +ifdef CONFIG_KUNIT
> > +obj-y += kunit/
> >  endif
>
> We actually have a pattern for this, although I guess it's rare enough
> that "pattern" isn't necessarily the right word.
>
> But you can find things like the Hyper-V drivers having similar
> issues, and so the driver Makefile has
>
>     obj-$(subst m,y,$(CONFIG_HYPERV))       += hv/
>
> See a few other cases with
>
>     git grep "subst m,y,"
>
> but I guess the "ifdef CONFIG_KUNIT" thing works too. I can only find
> one case of that (in arch/mips/Kbuild).
>
> Another way of dealing with this - that is more common for individual
> object files rather than directories - is to just do
>
>     kunit-dir-$(CONFIG_KUNIT) := kunit/
>     obj-y += $(kunit-dir-y) $(kunit-dir-m)
>
> which admittedly is also not a hugely common pattern, but does exist
> in various places (see for example the 'sfp-bus.o' file and CONFIG_SFP
> in drivers/net/phy/Makefile.
>
> That last pattern is probably most common in scripts/Makefile.lib,
> where we have things like
>
>        hostprogs += $(hostprogs-always-y) $(hostprogs-always-m)
>
> which is similar but not the exact same thing.
>
> Anyway, I guess I'll just apply that patch as-is, I just wanted to
> point out that the particular pattern it uses may be simple, but we've
> generally tried to just do our Makefile evaluations with "arithmetic"
> rather than conditionals.
>
>               Linus

Thanks, Linus.

(And also thanks to Mikhail Gavrilov and Thorsten Leemhuis for testing
this change as well.)

It's certainly been an educational experience!

Of those other options, personally I find the "subst m,y" one most
obvious, but I'll probably leave it as the conditional for now, unless
you think trying to make it more consistent is worth the extra churn.

Regardless, I'll look into adding a note about this to
Documentation/kbuild/makefiles.rst.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]