build: Ensure assym.inc and offset.inc are generated in proper order.

[• hselasky]

Summary

It has been observed kernel modules depending on these header files
may sporadically fail to build. By enforcing a build order, the
problem is solved.

MFC after: 1 week
Sponsored by: NVIDIA Networking

[imp]

Do you really need both? Before depend is how we do all the other ones like this... unless something has changed that I missed

[• hselasky]

@imp : Get me right: Maybe I need to += them to a new variable?

When you have "beforedepend:" inside a .if conditional, is that respected?

What I meant to code is:

When these files are needed, assym.inc and offset.inc, they need to happen beforedepend and beforebuild.

There is no problem when using -j1 . I only saw this occasionally when using -jXX .

--HPS

[kib]

Which modules need .inc files? For amd64, I cannot think about any except efirt.ko and might be linux.ko. You incur the overhead to generate the files to all modules, from my reading.

[imp]

In D40191#915079, @kib wrote:

Which modules need .inc files? For amd64, I cannot think about any except efirt.ko and might be linux.ko. You incur the overhead to generate the files to all modules, from my reading.

I thought all of the modules need offset.inc:

sys/systm.h:

#ifdef _KERNEL
...
#include <sys/kpilite.h>
...

sys/kpilite.h will use them if:

#if !defined(GENOFFSET) && (!defined(KLD_MODULE) || defined(KLD_TIED)) && defined(_KERNEL)
#include "offset.inc"

(note redundant check for _KERNEL). KLD_TIED is defined in

.if defined(MODULE_TIED)
CFLAGS+=        -DKLD_TIED
.endif

which means maybe all of them if you wanted to build a tied set of modules, though
since that's what we do in kern.post.mk:

MKMODULESENV+=  MODULE_TIED=yes

though if they are built as part of world or via some standalone route in ports they don't
get tied (unless I've missed something)... except drmkmod which I think requests tied build
in its makefiles (I'm too tired to chase things that far).

I'm still not sure offset.inc was a good idea, but replacing it is fraught.

[imp]

In D40191#915026, @hselasky wrote:

@imp : Get me right: Maybe I need to += them to a new variable?

When you have "beforedepend:" inside a .if conditional, is that respected?

What I meant to code is:

When these files are needed, assym.inc and offset.inc, they need to happen beforedepend and beforebuild.

There is no problem when using -j1 . I only saw this occasionally when using -jXX .

I was hoping you'd know why both were needed, since we have in bsd.dep.mk:

depend: beforedepend ${DEPENDFILE} afterdepend
...
beforebuild: depend

which would mean that beforedepend is always done before beforebuild. If you need them both,
then that sure sounds like something else is amiss that we should have @sjg look into.

[sjg]

I was going to say don't make beforebuild depend on depend

The DIRDEPS_BUILD uses beforebuild to trigger things (like generating and staging headers) before we try to compile anything
but it does not do any explicit depend processing which is where header generation etc is often triggered in the non-DIRDEPS_BUILD
So beforebuild: depend would be wrong.

[sjg]

I was hoping you'd know why both were needed, since we have in bsd.dep.mk:

FWIW bsd.dep.mk only does beforebuild: depend when MK_DIRDEPS_BUILD is "no".

depend: beforedepend ${DEPENDFILE} afterdepend
...
beforebuild: depend

which would mean that beforedepend is always done before beforebuild. If you need them both,
then that sure sounds like something else is amiss that we should have @sjg look into.

[sjg]

The patch as is should work fine.
Though it is obviously a little confusing why we have both beforedepend and beforebuild dependencies.
You *could* put offset.inc and assym.inc into a list (naming the list is the hard bit ;-)

genlist += assym.inc
..
.if ${MK_DIRDEPS_BUILD} == "yes"
beforebuild: ${genlist}
.else
befordepend: ${genlist}
.endif

the above (apart from the crappy list name) would perhaps make it more obvious why we have two possible dependencies.

Another alternative would be to just add a comment

[imp]

Ok. A comment about why is good for me.. I should have looked more deeply. Thanks @sjg...

[• hselasky]

The patch as is should work fine.

Though it is obviously a little confusing why we have both beforedepend and beforebuild dependencies.
You *could* put offset.inc and assym.inc into a list (naming the list is the hard bit 😉

I also thought using a variable would be more clear.

About both beforedepend and beforebuild, the reasoning was the following:

Aren't dependencies created after the first compilation?
And "make all" does not invoke "make depend".

In other words I concluded both targets need to depend on generating these files.

--HPS

[imp]

Your conclusion is valid only for meta mode. For non-meta mode, beforedepend is always buiilt before beforebuild, even if you aren't generating the .depend files. So your logic doesn't match the code, and having an expliict comment about why both is better. And that discitnction should be explicitly called out because otherwises it looks redundant to people that know the code and they might remove it if the justistifcaion isn't there with specificity (and not the logical conclusion, but the actual implementation). It tripped me up and I've been looking at this stuf for 20 years...

I'm not sure I agree an extra variable is needed here. I'm not sure what it advantge that has, but I'll see what you come up with there.