| Message ID | 20220808114908.240813-2-sv@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | objtool: Enable and implement --mcount option on powerpc | expand |
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. So maybe it would be better to leave that as is, and instead modify annotate_unreachable() so that only architectures requiring it absolutely get it. For instance by defining and using a new config item, for instance CONFIG_OBJTOOL_NEEDS_ANNOTATE_UNREACHABLE > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > arch/powerpc/include/asm/bug.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h > index 61a4736355c2..074be1a78c56 100644 > --- a/arch/powerpc/include/asm/bug.h > +++ b/arch/powerpc/include/asm/bug.h > @@ -99,7 +99,7 @@ > __label__ __label_warn_on; \ > \ > WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ > - unreachable(); \ > + __builtin_unreachable(); \ > \ > __label_warn_on: \ > break; \
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
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
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
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(). > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > arch/powerpc/include/asm/bug.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h > index 61a4736355c2..074be1a78c56 100644 > --- a/arch/powerpc/include/asm/bug.h > +++ b/arch/powerpc/include/asm/bug.h > @@ -99,7 +99,7 @@ > __label__ __label_warn_on; \ > \ > WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ > - unreachable(); \ > + __builtin_unreachable(); \ Should you add barrier_before_unreachable() before __builtin_unreachable() to avoid stack bombs ? See commit 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()") > \ > __label_warn_on: \ > break; \
Hi Christophe, On 26/08/22 15:48, 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(). >> >> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> >> --- >> arch/powerpc/include/asm/bug.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h >> index 61a4736355c2..074be1a78c56 100644 >> --- a/arch/powerpc/include/asm/bug.h >> +++ b/arch/powerpc/include/asm/bug.h >> @@ -99,7 +99,7 @@ >> __label__ __label_warn_on; \ >> \ >> WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ >> - unreachable(); \ >> + __builtin_unreachable(); \ > Should you add barrier_before_unreachable() before > __builtin_unreachable() to avoid stack bombs ? > > See commit 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()") Yes, adding barrier_before_unreachable() before __builtin_unreachable() works around the build issues reported at https://lkml.org/lkml/2022/8/25/418. I'll post a v2 with this change. Thanks for the suggestion! - Sathvika
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index 61a4736355c2..074be1a78c56 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -99,7 +99,7 @@ __label__ __label_warn_on; \ \ WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ - unreachable(); \ + __builtin_unreachable(); \ \ __label_warn_on: \ break; \
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(). Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> --- arch/powerpc/include/asm/bug.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)