Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 8 Apr 2022 17:48:21 -0700
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Michal Marek <michal.lkml@markovi.net>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Sam Ravnborg <sam@ravnborg.org>, X86 ML <x86@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Changbin Du <changbin.du@gmail.com>
Subject: Re: [PATCH] kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH
Message-ID: <20220409004821.tcztejr2cgsieqaq@treble>
References: <7fad83ecde03540e65677959034315f8fbb3755e.1649434832.git.jpoimboe@redhat.com>
 <CAK7LNARvFcQgEB1b0L6giwx0vD7wU9L-OZ5jvm1c5+StLjeOYQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAK7LNARvFcQgEB1b0L6giwx0vD7wU9L-OZ5jvm1c5+StLjeOYQ@mail.gmail.com>
Precedence: bulk

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.

> 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