On Mon, Jul 24, 2023 at 11:30 AM Arnd Bergmann <arnd@xxxxxxxxxx> wrote: > > On Mon, Jul 24, 2023, at 20:13, Arnd Bergmann wrote: > > >>> 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. > >> > >> Which gcc are we talking about here that is so buggy? > > > > I think I only tried versions 8 through 13 for this one, but > > can check others as well. > > I have a minimized test case at https://godbolt.org/z/hK4ev17fv > that shows the problem happening with all versions of gcc > (4.1 through 14.0) if I force the dec_active() function to be > inline and force inc_active() to be non-inline. That's a bit of cheating, but I see your point now. How about we do: diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c index 51d6389e5152..3fa0944cb975 100644 --- a/kernel/bpf/memalloc.c +++ b/kernel/bpf/memalloc.c @@ -183,11 +183,11 @@ static void inc_active(struct bpf_mem_cache *c, unsigned long *flags) WARN_ON_ONCE(local_inc_return(&c->active) != 1); } -static void dec_active(struct bpf_mem_cache *c, unsigned long flags) +static void dec_active(struct bpf_mem_cache *c, unsigned long *flags) { local_dec(&c->active); if (IS_ENABLED(CONFIG_PREEMPT_RT)) - local_irq_restore(flags); + local_irq_restore(*flags); } static void add_obj_to_free_list(struct bpf_mem_cache *c, void *obj) @@ -197,7 +197,7 @@ static void add_obj_to_free_list(struct bpf_mem_cache *c, void *obj) inc_active(c, &flags); __llist_add(obj, &c->free_llist); c->free_cnt++; - dec_active(c, flags); + dec_active(c, &flags); It's symmetrical and there is no 'flags = 0' ugliness.
