Hi Xin,
On Sun, Oct 16, 2022 at 06:28:27AM +0000, Li, Xin3 wrote:
> Arch maintainers,
>
> We plan to upgrade the orphan section warning to a hard link error on x86.
> And I found the most recent related commit 59612b24f78a0
> ("kbuild: Hoist '--orphan-handling' into Kconfig") from by Nathan Chancellor
> has the following statement in the help section:
>
> +config ARCH_WANT_LD_ORPHAN_WARN
> + bool
> + help
> + An arch should select this symbol once all linker sections are explicitly
> + included, size-asserted, or discarded in the linker scripts. This is
> + important because we never want expected sections to be placed heuristically
> + by the linker, since the locations of such sections can change between linker
> + versions.
> +
+ Kees, who did the heavy lifting to enable '--orphan-handling=warn' for
arm64 and x86, and the ClangBuiltLinux and linux-kbuild mailing lists.
Unfortunately, for some reason, I do not see the original posting on
lore but I left the full message intact for further discussion.
> It looks to me that it actually suggests a link error rather than a warning,
> so the question is, should we do the upgrade on all architectures with
> the orphan section warning?
It might be interesting to turn orphan sections into an error if
CONFIG_WERROR is set. Perhaps something like the following (FYI, not
even compile tested)?
diff --git a/Makefile b/Makefile
index 0837445110fc..485f47fc2c07 100644
--- a/Makefile
+++ b/Makefile
@@ -1119,7 +1119,7 @@ endif
# We never want expected sections to be placed heuristically by the
# linker. All sections should be explicitly named in the linker script.
ifdef CONFIG_LD_ORPHAN_WARN
-LDFLAGS_vmlinux += --orphan-handling=warn
+LDFLAGS_vmlinux += --orphan-handling=$(if $(CONFIG_WERROR),error,warn)
endif
# Align the bit size of userspace programs with the kernel
Outright turning the warning into an error with no escape hatch might be
too aggressive, as we have had these warnings triggered by new compiler
generated sections, such as in commit 848378812e40 ("vmlinux.lds.h:
Handle clang's module.{c,d}tor sections"). Unconditionally breaking the
build in these situations is unfortunate but the warnings do need to be
dealt with so I think having it error by default with the ability to
opt-out is probably worth doing. I do not have a strong opinion though.
> BTW, the following architectures enable orphan section warning,
> arm/arm64/hexagon/loongarch/mips/ powerpc/x86,
> while all other architectures just ignore it.
Right, every architecture should eventually select
CONFIG_ARCH_WANT_LD_ORPHAN_WARN so that they get this warning as well.
It is just making architecture maintainers aware of it so they can look
into it.
Cheers,
Nathan
On Mon, Oct 17, 2022 at 11:26:47AM -0700, Nathan Chancellor wrote:
> It might be interesting to turn orphan sections into an error if
> CONFIG_WERROR is set. Perhaps something like the following (FYI, not
> even compile tested)?
>
> diff --git a/Makefile b/Makefile
> index 0837445110fc..485f47fc2c07 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1119,7 +1119,7 @@ endif
> # We never want expected sections to be placed heuristically by the
> # linker. All sections should be explicitly named in the linker script.
> ifdef CONFIG_LD_ORPHAN_WARN
> -LDFLAGS_vmlinux += --orphan-handling=warn
> +LDFLAGS_vmlinux += --orphan-handling=$(if $(CONFIG_WERROR),error,warn)
> endif
Yes, this is much preferred.
> Outright turning the warning into an error with no escape hatch might be
> too aggressive, as we have had these warnings triggered by new compiler
> generated sections, such as in commit 848378812e40 ("vmlinux.lds.h:
> Handle clang's module.{c,d}tor sections"). Unconditionally breaking the
> build in these situations is unfortunate but the warnings do need to be
> dealt with so I think having it error by default with the ability to
> opt-out is probably worth doing. I do not have a strong opinion though.
Correct; the mandate from Linus (disregarding his addition of
CONFIG_WERROR for all*config builds), is that we should avoid breaking
builds. It wrecks bisection, it causes problems across compiler versions,
etc.
So, yes, only on CONFIG_WERROR=y.
--
Kees Cook
On Mon, Oct 17, 2022 at 12:32:39PM -0700, Kees Cook wrote:
> On Mon, Oct 17, 2022 at 11:26:47AM -0700, Nathan Chancellor wrote:
> > It might be interesting to turn orphan sections into an error if
> > CONFIG_WERROR is set. Perhaps something like the following (FYI, not
> > even compile tested)?
> >
> > diff --git a/Makefile b/Makefile
> > index 0837445110fc..485f47fc2c07 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1119,7 +1119,7 @@ endif
> > # We never want expected sections to be placed heuristically by the
> > # linker. All sections should be explicitly named in the linker script.
> > ifdef CONFIG_LD_ORPHAN_WARN
> > -LDFLAGS_vmlinux += --orphan-handling=warn
> > +LDFLAGS_vmlinux += --orphan-handling=$(if $(CONFIG_WERROR),error,warn)
> > endif
>
> Yes, this is much preferred.
>
> > Outright turning the warning into an error with no escape hatch might be
> > too aggressive, as we have had these warnings triggered by new compiler
> > generated sections, such as in commit 848378812e40 ("vmlinux.lds.h:
> > Handle clang's module.{c,d}tor sections"). Unconditionally breaking the
> > build in these situations is unfortunate but the warnings do need to be
> > dealt with so I think having it error by default with the ability to
> > opt-out is probably worth doing. I do not have a strong opinion though.
>
> Correct; the mandate from Linus (disregarding his addition of
> CONFIG_WERROR for all*config builds), is that we should avoid breaking
> builds. It wrecks bisection, it causes problems across compiler versions,
> etc.
>
> So, yes, only on CONFIG_WERROR=y.
We would probably want to alter the text of CONFIG_WERROR in some manner
to convey this, perhaps like so:
diff --git a/init/Kconfig b/init/Kconfig
index a19314933e54..1fc03e4b2af2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -165,10 +165,12 @@ config WERROR
help
A kernel build should not cause any compiler warnings, and this
enables the '-Werror' (for C) and '-Dwarnings' (for Rust) flags
- to enforce that rule by default.
+ to enforce that rule by default. Certain warnings from other tools
+ such as the linker may be upgraded to errors with this option as
+ well.
- However, if you have a new (or very old) compiler with odd and
- unusual warnings, or you have some architecture with problems,
+ However, if you have a new (or very old) compiler or linker with odd
+ and unusual warnings, or you have some architecture with problems,
you may need to disable this config option in order to
successfully build the kernel.
---
Cheers,
Nathan