Page MenuHomeFreeBSD

LinuxKPI: rework module build options to use CFLAGS_LINUXKPI=
Needs ReviewPublic

Authored by bz on Thu, Oct 1, 6:01 PM.

Details

Reviewers
None
Group Reviewers
linuxkpi
Summary

While trying to add extra locking debugging to LinuxKPI it became
apparent that the order in which we process CFLAGS+= with the so far
common ${LINUXKPI_INCLUDES} does not work as it will add the
LinuxKPI specifc -include for kconfig.h before the global -inlcude
for opt_global.h. This meant that a #if defined(WITNESS) check in
kconfig.h would always fail given the include order.

Further we do have various LinuxKPI modules, which will add CFLAGS+=
after including kmod.mk. It is unclear as to which extend that was
a copy and paste problem or a real issue and will have to be
investigated independent of this change.

Rework the way we add ${LINUXKPI_INCLUDES} to the build by adding a
CFLAGS_LINUXKPI= options to the Makefiles and removing the manual
CFLAGS+= lines. Then in kmod.mk make sure that LinuxKPI options
are added after the global options.

CFLAGS_LINUXKPI can either be defined empty as CFLAGS_LINUXKPI= ,
or set to YES (a common way of expressing options such as EXPORT_SYMS=),
or add further CFLAGS+= which will be added after the global and the
generic LinuxKPI CFLAGS.

It should be noteds that for code which is not affected or does
not affect kernel build options normal CFLAGS+= can be kept for all
other -I or -D lines. It will be the responsibility of the driver
maintainers (of semi-native drivers) to see if current way of handling
includes may have (had) unexpected effects.

Out-of-tree consumers should update their module build frameworks
accordingly.

Fixes: 41283b454b7a ("LinuxKPI: always include linux/kconfig.h")
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 77582
Build 74465: arc lint + arc unit

Event Timeline

bz requested review of this revision.Thu, Oct 1, 6:01 PM

The logic seems sound but somehow CFLAGS_LINUXKPI is odd to me, like CFLAGS_* should contain flags rather than be a boolean. E.g. we have CFLAGS_NO_SIMD= -mno-mmx -mno-sse...
Hrm, but we also have e.g. SSP_CFLAGS?= -fstack-protector-strong -fstack-clash-protection and that form is more common.
ADD_LINUXKPI_CFLAGS?

The logic seems sound but somehow CFLAGS_LINUXKPI is odd to me, like CFLAGS_* should contain flags rather than be a boolean. E.g. we have CFLAGS_NO_SIMD= -mno-mmx -mno-sse...
Hrm, but we also have e.g. SSP_CFLAGS?= -fstack-protector-strong -fstack-clash-protection and that form is more common.
ADD_LINUXKPI_CFLAGS?

I was looking at the *EXTRA versions we have initially and figured we do use suffixes in other parts like MODULES_EXTRA, _COPTFLAGS_EXTRA, CWARNEXTRA (no _), KERNEL_EXTRA. But CFLAGS_EXTRA wouldn't work so it became CFLAGS_LINUXKPI.

Maybe just LINUXKPI_CFLAGS without the ADD keeping the "LINUXKPI" name space like in LINUXKPI_GENSRC and LINUXKPI_INCLUDES?
Then I'd likely remove the LINUXKPI_CFLAGS= version from the docs and make sure it is set to something at least (YES or -I...)?

In the end I'll do the s///g to whatever people think is best.

In the end I'll do the s///g to whatever people think is best.

Unfortunately I don't have a great idea of a better name, it just looked a little odd. So I suspect you'll have to pick something and nobody will object. If you want to leave it as is that's fine with me too. It looks a bit strange but it's obvious in context that we're not literally adding "YES" to CFLAGS.

I was going to say that e.g. LINUXKPI_CFLAGS=YES would be my suggestion before discovering that most of the literal CFLAGS additions use that form already. I do like that it's in a LINUXKPI namespace though.