Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()

[Naveen N. Rao]

Christophe Leroy wrote:
> Le 30/06/2022 à 10:05, Naveen N. Rao a écrit :
> > Christophe Leroy wrote:
> > > > The builtin variant of unreachable (__builtin_unreachable()) works.
> > > >
> > > > How about using that instead of unreachable() ?
> > >
> > > In fact the problem comes from the macro annotate_unreachable() which 
> > > is called by unreachable() before calling __build_unreachable().
> > >
> > > Seems like this macro adds (after the unconditional trap twui) a call 
> > > to an empty function whose address is listed in section 
> > > .discard.unreachable
> > >
> > >      1c78:       00 00 e0 0f     twui    r0,0
> > >      1c7c:       55 e7 ff 4b     bl      3d0 
> > > <qdisc_root_sleeping_lock.part.0>
> > >
> > >
> > > RELOCATION RECORDS FOR [.discard.unreachable]:
> > > OFFSET           TYPE              VALUE
> > > 0000000000000000 R_PPC64_REL32     .text+0x00000000000003d0
> > >
> > > The problem is that that function has size 0:
> > >
> > > 00000000000003d0 l     F .text    0000000000000000 
> > > qdisc_root_sleeping_lock.part.0
> > >
> > >
> > > And objtool is not prepared for a function with size 0.
> >
> > annotate_unreachable() seems to have been introduced in commit 
> > 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead 
> > ends").
> >
> > Objtool considers 'ud2' instruction to be fatal, so BUG() has 
> > __builtin_unreachable(), rather than unreachable(). See commit 
> > bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS() 
> > asm"). For the same reason, __WARN_FLAGS() is annotated with 
> > _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
> >
> > On powerpc, we use trap variants for both and don't have a special 
> > instruction for a BUG(). As such, for _WARN_FLAGS(), using 
> > __builtin_unreachable() suffices to achieve optimal code generation from 
> > the compiler. Objtool would consider subsequent instructions to be 
> > reachable. For BUG(), we can continue to use unreachable() so that 
> > objtool can differentiate these from traps used in warnings.
>
> Not sure I understand what you mean.
>
> __WARN_FLAGS() and BUG() both use 'twui' which is unconditionnal trap, 
> as such both are the same.
>
> On the other side, WARN_ON() and BUG_ON() use tlbnei which is a 
> conditionnel trap.

Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
BUG(), there is no need for an annotation since objtool assumes that 
'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
used, an explicit annotate_reachable() is needed. That's _reachable_, to 
indicate that the control flow can continue with the next instruction.

On powerpc, we should (eventually) classify all trap variants as 
INSN_TRAP. Even in the absence of that classification today, objtool 
assumes that control flow continues with the next instruction. With your 
work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
being generated, I think it is appropriate to just use 
__builtin_unreachable() and to not use the annotation.

In any case, we are only hitting this since gcc is generating a 'bl' due 
to that annotation. We are not yet enabling full objtool validation on 
powerpc, so I think we can revisit this at that point.

> > > The following changes to objtool seem to fix the problem, most warning 
> > > are gone with that change.
> > >
> > > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > > index 63218f5799c2..37c0a268b7ea 100644
> > > --- a/tools/objtool/elf.c
> > > +++ b/tools/objtool/elf.c
> > > @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
> > > struct rb_node *node)
> > >
> > >       if (*o < s->offset)
> > >           return -1;
> > > +    if (*o == s->offset && !s->len)
> > > +        return 0;
> > >       if (*o >= s->offset + s->len)
> > >           return 1;
> > >
> > > @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, struct 
> > > symbol *sym)
> > >        * Don't store empty STT_NOTYPE symbols in the rbtree.  They
> > >        * can exist within a function, confusing the sorting.
> > >        */
> > > -    if (!sym->len)
> > > +    if (sym->type == STT_NOTYPE && !sym->len)
> > >           rb_erase(&sym->node, &sym->sec->symbol_tree);
> > >   }
> >
> > Is there a reason to do this, rather than change __WARN_FLAGS() to use 
> > __builtin_unreachable()? Or, are you seeing an issue with unreachable() 
> > elsewhere in the kernel?
>
> At the moment I'm trying to understand what the issue is, and explore 
> possible fixes. I guess if we tell objtool that after 'twui' subsequent 
> instructions are unreachable, then __builtin_unreachable() is enough.

Yes, see my explanation above. Since no 'bl' is emitted with the 
builtin, objtool won't complain, especially for mcount.

> I think we should also understand why annotate_unreachable() gives us a 
> so bad result and see if it can be changed to something cleaner than a 
> 'bl' to an empty function that has no instructions.

Indeed. Not really sure. annotate_unreachable() wants to take the 
address of the instruction after the trap. But, in reality, due to use 
of asm goto for __WARN_FLAGS, no instructions would be generated. I 
wonder if that combination causes such code to be emitted.


- Naveen