lkml.org 
[lkml]   [2022]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH
On Fri, Apr 8, 2022 at 5:48 PM Josh Poimboeuf <jpoimboe@redhat.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

\
 
 \ /
  Last update: 2022-04-20 19:05    [W:0.100 / U:0.776 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site