Messages in this thread | | | Date | Thu, 18 Aug 2022 17:55:04 +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 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@csgroup.eu
Peter was of the opinion that INSN_TRAP may not be what we want: http://lkml.kernel.org/r/YsLSU6idNME/BtwH@hirez.programming.kicks-ass.net
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
| |