On Fri, Apr 8, 2022 at 5:48 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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@xxxxxxxxxx/ 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
