Message-ID: <CAKwvOdmfUDZPyhtF5fvdKy7i-qRKB7Bmq80DhP0F=yarG+rnpw@mail.gmail.com>
Date:   Wed, 20 Apr 2022 10:03:18 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Sam Ravnborg <sam@...nborg.org>, X86 ML <x86@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Changbin Du <changbin.du@...il.com>
Subject: Re: [PATCH] kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH

On Fri, Apr 8, 2022 at 5:48 PM Josh Poimboeuf <jpoimboe@...hat.com> wrote:
>
> On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote:
> > > But with -O2, once-called static functions are almost always inlined, so
> >
> > "always inlined" per GCC manual.
> >  -O2, -O3, -O3 enables  -finline-functions-called-once
>
> Not necessarily.  The manual also says:
>
>        -finline-functions-called-once
>            Consider all "static" functions called once for inlining into
>            their caller even if they are not marked "inline".  If a call
>            to a given function is integrated, then the function is not
>            output as assembler code in its own right.
>
> So it "considers" inlining them, but it doesn't guarantee it.  And when
> I looked at the GCC code some months ago I remember seeing a few edge
> cases where it would inline.
>
> > > its usefulness for rooting out mismatch warnings on other configs is
> > > somewhere between extremely limited and non-existent.  And nowadays we
> > > have build bots everywhere doing randconfigs continuously, which are
> > > great for rooting out such edge cases.
> > >
> > > Somewhat ironically, the existence of those build bots means we get a
> > > lot of unrealistic objtool warnings being reported, due to unrealistic
> > > inlines caused by CONFIG_DEBUG_SECTION_MISMATCH, where the only way to
> > > silence the warnings is to force a single-called function to be inlined
> > > with '__always_inline'.
> > >
> > > So the limited, hypothetical benefit of "rooting out configs with
> > > section mismatches" is outweighed by the very real downside of "adding
> > > lots of unnecessary '__always_inline' annotations".
> >
> >
> > I am confused with the description because
> > you are talking about two different warnings.
> >
> > [1]  modpost warning  (section mismatch)
> > [2]  objtool warnings
>
> It's all a bit confusing.
>
> To clarify: in theory, the point of CONFIG_DEBUG_SECTION_MISMATCH was to
> trigger more *modpost* warnings.  (It may do that, but the extra
> warnings are probably not realistic.  And even if they were realistic on
> some configs, they would be found by modpost warnings on those configs
> found by build bots.)
>
> But CONFIG_DEBUG_SECTION_MISMATCH is also triggering more *objtool*
> warnings, which is a problem because the warnings are not realistic...
>
> > For [1], you can add __init marker to the callers,
> > and that is the right thing to do.
>
> Yes, either add __init to the caller or remove __init from the callee.
>
> But even if such "inlined single caller" modpost warnings correspond to
> real world configs (which is very questionable) then we'd expect them to
> show up in real-world randconfig bot testing, without having the need
> for CONFIG_DEBUG_SECTION_MISMATCH to root out the rare edge cases.
>
> > Is [2] caused by dead code that was not optimized out
> > due to the unusual inlining decisions by the compiler ?
>
> As Peter mentioned it was due to validation of SMAP uaccess rules.
>
> > If the issues are all about objtool,
> > "depends on !STACK_VALIDATION" might be
> > an alternative approach?
> >
> > config DEBUG_SECTION_MISMATCH
> >            bool "Enable full Section mismatch analysis"
> >            depends on CC_IS_GCC
> >            depends on !STACK_VALIDATION        # do not confuse objtool
>
> Yes, that would be another way to handle it.  Though that means the
> option would effectively be dead on x86-64.

Does this series help (or is it related to this thread)?
https://lore.kernel.org/lkml/cover.1650300597.git.jpoimboe@redhat.com/
Patch 17/25 seems to make STACK_VALIDATION unwinder-dependent (on
CONFIG_UNWINDER_FRAME_POINTER)?

>
> > Now that -Og was already rejected, the possible flag for building the kernel is
> > -O2, O3, -Os.
> > All of them enable -finline-functions-called-once.
> >
> > So, there is no practical case for -fno-inline-functions-called-once
> > unless we explicitly enable it.
>
> Agreed.
>
> > I am OK with this patch, I am still wondering if this could be useful
> > to detect missing __init markers.
>
> So there's two ways to look at this question: at a source level and at a
> binary level.
>
> At a source level, if there's a non-init function which inlines a
> single-called __init function, and modpost doesn't notice it (because
> the __init function doesn't access any __init symbols), then the __init
> function wrongly has the __init annotation.  But calling that a bug is
> very questionable, because it's not a real bug in the binary.  IMO it's
> not worth worrying about, when we have real bugs to fix.
>
> At a binary level, if there's a real section mismatch bug with a given
> config, modpost will warn about it.  This option doesn't affect that.
>
> --
> Josh
>


-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists