From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
Sathvika Vasireddy <sv@linux.ibm.com>
Cc: "aik@ozlabs.ru" <aik@ozlabs.ru>,
"chenzhongjin@huawei.com" <chenzhongjin@huawei.com>,
"jpoimboe@redhat.com" <jpoimboe@redhat.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mbenes@suse.cz" <mbenes@suse.cz>,
"mingo@redhat.com" <mingo@redhat.com>,
"mpe@ellerman.id.au" <mpe@ellerman.id.au>,
"npiggin@gmail.com" <npiggin@gmail.com>,
"peterz@infradead.org" <peterz@infradead.org>,
"rostedt@goodmis.org" <rostedt@goodmis.org>
Subject: Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin variant in WARN_ON()
Date: Thu, 18 Aug 2022 16:16:32 +0530 [thread overview]
Message-ID: <1660817468.4x4re2ul0k.naveen@linux.ibm.com> (raw)
In-Reply-To: <82eec792-b71f-17cc-d905-368fd5ca62f2@csgroup.eu>
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
next prev parent reply other threads:[~2022-08-18 10:47 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-08 11:48 [PATCH 00/16] objtool: Enable and implement --mcount option on powerpc Sathvika Vasireddy
2022-08-08 11:48 ` [PATCH 01/16] powerpc: Replace unreachable() with it's builtin variant in WARN_ON() Sathvika Vasireddy
2022-08-10 8:28 ` Christophe Leroy
2022-08-18 10:46 ` Naveen N. Rao [this message]
2022-08-18 11:06 ` Christophe Leroy
2022-08-18 12:25 ` Naveen N. Rao
2022-08-26 10:18 ` Christophe Leroy
2022-08-29 5:46 ` Sathvika Vasireddy
2022-08-08 11:48 ` [PATCH 02/16] powerpc: override __ALIGN() and __ALIGN_STR() macros Sathvika Vasireddy
2022-08-10 8:31 ` Christophe Leroy
2022-08-18 10:46 ` Sathvika Vasireddy
2022-08-08 11:48 ` [PATCH 03/16] powerpc: Fix objtool unannotated intra-function call warnings Sathvika Vasireddy
2022-08-10 8:32 ` Christophe Leroy
2022-08-18 10:55 ` Sathvika Vasireddy
2022-08-08 11:48 ` [PATCH 04/16] powerpc: curb " Sathvika Vasireddy
2022-08-08 11:48 ` [PATCH 05/16] powerpc: Skip objtool from running on drivers/crypto/vmx/aesp8-ppc.o Sathvika Vasireddy
2022-08-10 8:34 ` Christophe Leroy
2022-08-18 11:12 ` Sathvika Vasireddy
2022-08-08 11:48 ` [PATCH 06/16] powerpc: Fix objtool unannotated intra-function call warnings on PPC32 Sathvika Vasireddy
2022-08-09 23:31 ` kernel test robot
2022-08-08 11:48 ` [PATCH 07/16] powerpc: Skip objtool from running on VDSO files Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 08/16] objtool: Fix SEGFAULT Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 09/16] objtool: Use target file endianness instead of a compiled constant Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 10/16] objtool: Use target file class size " Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 11/16] objtool: Add --mnop as an option to --mcount Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 12/16] objtool: Read special sections with alts only when specific options are selected Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 13/16] objtool: Use macros to define arch specific reloc types Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 14/16] objtool: Add arch specific function arch_ftrace_match() Sathvika Vasireddy
2022-08-12 10:58 ` kernel test robot
2022-08-13 13:48 ` kernel test robot
2022-08-08 11:49 ` [PATCH 15/16] objtool/powerpc: Enable objtool to be built on ppc Sathvika Vasireddy
2022-08-08 11:49 ` [PATCH 16/16] objtool/powerpc: Add --mcount specific implementation Sathvika Vasireddy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1660817468.4x4re2ul0k.naveen@linux.ibm.com \
--to=naveen.n.rao@linux.vnet.ibm.com \
--cc=aik@ozlabs.ru \
--cc=chenzhongjin@huawei.com \
--cc=christophe.leroy@csgroup.eu \
--cc=jpoimboe@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mbenes@suse.cz \
--cc=mingo@redhat.com \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=sv@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).