On Thu, Oct 20, 2022 at 05:17:35AM +0000, Li, Xin3 wrote:
> Hi Nathan,
>
> > 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.
>
> Thanks a lot for making this crystal clear.
>
> Do you want me to continue? Or maybe it's easier for you to complete it?
Sure, I think it is reasonable for you to continue with this as you
brought up the idea initially! Feel free to just take those diffs
wholesale if they work and stick a
Suggested-by: Nathan Chancellor <[email protected]>
or
Co-developed-by: Nathan Chancellor <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
on the patch if you are so inclined or rework them in a way you see fit,
I do not have a strong opinion.
> I will need to find resources to test the patch on other platforms besides x86.
In theory, we should have already cleaned up all these warnings when we
enabled CONFIG_LD_ORPHAN_WARN for all these architectures, so that
change should be a no-op. More testing is never a bad idea though :)
I can throw it into my LLVM testing matrix as well.
Cheers,
Nathan