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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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 ?

Christophe