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
