Messages in this thread Patches in this message | | | Date | Thu, 18 Aug 2022 16:16:32 +0530 | From | "Naveen N. Rao" <> | Subject | Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin variant in WARN_ON() |
| |
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.
- Naveen
| |