Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 Aug 2022 17:55:04 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin
 variant in WARN_ON()
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>
References: <20220808114908.240813-1-sv@linux.ibm.com>
        <20220808114908.240813-2-sv@linux.ibm.com>
        <82eec792-b71f-17cc-d905-368fd5ca62f2@csgroup.eu>
        <1660817468.4x4re2ul0k.naveen@linux.ibm.com>
        <06b7a93b-5148-4d92-0b56-5956afdfd3fb@csgroup.eu>
In-Reply-To: <06b7a93b-5148-4d92-0b56-5956afdfd3fb@csgroup.eu>
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1660824799.vnjff6w3m0.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

Christophe Leroy wrote:
>=20
>=20
> Le 18/08/2022 =C3=A0 12:46, Naveen N. Rao a =C3=A9crit=C2=A0:
>> Christophe Leroy wrote:
>>>
>>>
>>> Le 08/08/2022 =C3=A0 13:48, Sathvika Vasireddy a =C3=A9crit=C2=A0:
>>>> 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=20
>>> necessary for powerpc, or even why using unreachable() is wrong.
>>>
>>> Allthough we are getting rid of the problem here by replacing=20
>>> unreachable() by __builtin_unreachable(), it might still be a problem=20
>>> in core parts of kernel which still use unreachable.
>>=20
>> I did a kernel build with this series applied, with a variant of=20
>> ppc64le_defconfig. I then did another build with the same config, but=20
>> with the below hunk to disable objtool:
>>=20
>> 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
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_MOD_ARCH_SPECIFIC
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_NMI=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if PERF_EVENTS || (PPC6=
4=20
>> && PPC_BOOK3S)
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_OPTPROBES
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_OBJTOOL=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if PPC32 || MPROFILE_KERNEL
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_OBJTOOL_MCOUNT=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if=
 HAVE_OBJTOOL
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_PERF_EVENTS
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_PERF_EVENTS_NMI=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if PPC64
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select HAVE_PERF_REGS
>>=20
>> This has the effect of disabling annotations for unreachable().
>>=20
>> When I compared the resulting object files, I did not see changes in=20
>> codegen relating to the annotation, like we do with using unreachable()=
=20
>> in __WARN_FLAGS().
>>=20
>> More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load()=
=20
>> uses BUG(), and the generated code remains the same with/without the=20
>> unreachable() annotation.
>>=20
>> This suggests that the bad codegen we are seeing with the annotation in=
=20
>> unreachable() is limited to its use in __WARN_FLAGS(), which I suspect=20
>> is due to an interaction with the use of asm_volatile_goto() for=20
>> WARN_ENTRY().
>>=20
>> If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes=20
>> before _some_ function in this object file, which happens to hold a=20
>> relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc=
=20
>> either emits no new instruction for the annotation, or a 'nop' in some=20
>> cases.
>>=20
>> If I add a 'nop' between WARN_ENTRY() and unreachable() in=20
>> __WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use=
=20
>> of asm_volatile_goto(), the problem goes away and no bl is emitted:
>>=20
>> diff --git a/arch/powerpc/include/asm/bug.h=20
>> 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 @@
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __label__ __label_warn_on;=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 \
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 \
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WARN_ENTRY("twi 31, 0, 0", BUGFLAG=
_WARNING | (flags),=20
>> __label_warn_on); \
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __asm__ __volatile__("nop");=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 \
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unreachable();=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 \
>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 \
>> __label_warn_on:
>>=20
>>=20
>> In summary, I think the annotation itself is fine and we are only seeing=
=20
>> an issue with its usage after WARN_ENTRY() due to use of=20
>> asm_volatile_goto. Other uses of unreachable() don't seem to exhibit=20
>> this problem.
>>=20
>> As such, I think this patch is appropriate for this series, though I=20
>> think we should capture some of this information in the changelog.
>>=20
>> Note also that if and when we start utlizing the annotation, if we=20
>> classify twui as INSN_BUG, this change will continue to be appropriate.
>>=20
>=20
> 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=20
flow here without the need for an annotation. Parsing extable will=20
then show that control flow continues with the label subsequently.


- Naveen