Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin variant in WARN_ON()

[Naveen N. Rao]

Christophe Leroy wrote:
> Le 18/08/2022 à 12:46, Naveen N. Rao a écrit :
> > Christophe Leroy wrote:
> > > Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :
> > > > objtool is throwing *unannotated intra-function call*
> > > > warnings with a few instructions that are marked
> > > > unreachable. Replace unreachable() with __builtin_unreachable()
> > > > to fix these warnings, as the codegen remains same
> > > > with unreachable() and __builtin_unreachable().
> > >
> > > I think it is necessary to explain why using unreachable() is not 
> > > necessary for powerpc, or even why using unreachable() is wrong.
> > >
> > > Allthough we are getting rid of the problem here by replacing 
> > > unreachable() by __builtin_unreachable(), it might still be a problem 
> > > in core parts of kernel which still use unreachable.
> >
> > I did a kernel build with this series applied, with a variant of 
> > ppc64le_defconfig. I then did another build with the same config, but 
> > with the below hunk to disable objtool:
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 6be2e68fa9eb64..4c466acdc70d4c 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -237,8 +237,6 @@ config PPC
> >         select HAVE_MOD_ARCH_SPECIFIC
> >         select HAVE_NMI                         if PERF_EVENTS || (PPC64 
> > && PPC_BOOK3S)
> >         select HAVE_OPTPROBES
> > -       select HAVE_OBJTOOL                     if PPC32 || MPROFILE_KERNEL
> > -       select HAVE_OBJTOOL_MCOUNT              if HAVE_OBJTOOL
> >         select HAVE_PERF_EVENTS
> >         select HAVE_PERF_EVENTS_NMI             if PPC64
> >         select HAVE_PERF_REGS
> >
> > This has the effect of disabling annotations for unreachable().
> >
> > When I compared the resulting object files, I did not see changes in 
> > codegen relating to the annotation, like we do with using unreachable() 
> > in __WARN_FLAGS().
> >
> > More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load() 
> > uses BUG(), and the generated code remains the same with/without the 
> > unreachable() annotation.
> >
> > This suggests that the bad codegen we are seeing with the annotation in 
> > unreachable() is limited to its use in __WARN_FLAGS(), which I suspect 
> > is due to an interaction with the use of asm_volatile_goto() for 
> > WARN_ENTRY().
> >
> > If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes 
> > before _some_ function in this object file, which happens to hold a 
> > relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc 
> > either emits no new instruction for the annotation, or a 'nop' in some 
> > cases.
> >
> > If I add a 'nop' between WARN_ENTRY() and unreachable() in 
> > __WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use 
> > of asm_volatile_goto(), the problem goes away and no bl is emitted:
> >
> > diff --git a/arch/powerpc/include/asm/bug.h 
> > b/arch/powerpc/include/asm/bug.h
> > index 61a4736355c244..88e0027c20ba5c 100644
> > --- a/arch/powerpc/include/asm/bug.h
> > +++ b/arch/powerpc/include/asm/bug.h
> > @@ -99,6 +99,7 @@
> >         __label__ __label_warn_on;                              \
> >                                                                 \
> >         WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), 
> > __label_warn_on); \
> > +       __asm__ __volatile__("nop");                            \
> >         unreachable();                                          \
> >                                                                 \
> > __label_warn_on:
> >
> >
> > In summary, I think the annotation itself is fine and we are only seeing 
> > an issue with its usage after WARN_ENTRY() due to use of 
> > asm_volatile_goto. Other uses of unreachable() don't seem to exhibit 
> > this problem.
> >
> > As such, I think this patch is appropriate for this series, though I 
> > think we should capture some of this information in the changelog.
> >
> > Note also that if and when we start utlizing the annotation, if we 
> > classify twui as INSN_BUG, this change will continue to be appropriate.
>
> INSN_TRAP instead of INSN_BUG ?

INSN_BUG, in line with your suggestion here:
http://lkml.kernel.org/r/ff623097-9f18-3914-5eae-bc6e4cd1510f@xxxxxxxxxx

Peter was of the opinion that INSN_TRAP may not be what we want:
http://lkml.kernel.org/r/YsLSU6idNME/BtwH@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

If we classify twui as INSN_BUG, then objtool will know to stop control 
flow here without the need for an annotation. Parsing extable will 
then show that control flow continues with the label subsequently.


- Naveen