Re: [PATCH] bpf: force inc_active()/dec_active() to be inline functions

["Arnd Bergmann" <arnd@arndb.de>]

On Sun, Jul 23, 2023, at 18:46, Alexei Starovoitov wrote:
> On Sun, Jul 23, 2023 at 7:25 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>> On Sat, Jul 22, 2023 at 3:48 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> > From: Arnd Bergmann <arnd@arndb.de>
>> >
>> > Splitting these out into separate helper functions means that we
>> > actually pass an uninitialized variable into another function call
>> > if dec_active() happens to not be inlined, and CONFIG_PREEMPT_RT
>> > is disabled:
>>
>> Do you mean that the compiler can remove the flags automatically when
>> dec_active() is inlined, but can't remove it automatically when
>> dec_active() is not inlined ?

My educated guess is that it's fine when neither of them are inlined,
since then gcc can assume that 'flags' gets initialized by
inc_active(), and it's fine when both are inlined since dead code
elimination then gets rid of both the initialization and the use.

The only broken case should be when inc_active() is inlined and
gcc can tell that there is never an initialization, but 
dec_active() is not inlined, so gcc assumes it is actually used.

>> If so, why can't we improve the compiler ?
>
> Agree.
> Sounds like a compiler bug.

I don't know what you might want to change in the compiler
to avoid this. Compilers are free to decide which functions to
inline in the absence of noinline or always_inline flags.

One difference between gcc and clang is that gcc tries to
be smart about warnings by using information from inlining
to produce better warnings, while clang never uses information
across function boundaries for generated warnings, so it won't
find this one, but also would ignore an unconditional use
of the uninitialized variable. 

>> If we have to change the kernel, what about the change below?
>
> To workaround the compiler bug we can simply init flag=0 to silence
> the warn, but even that is silly. Passing flag=0 into irqrestore is buggy.

Maybe inc_active() could return the flags instead of modifying
the stack variable? that would also result in slightly better
code when it's not inlined.

     Arnd