[RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()
Chen Zhongjin
chenzhongjin at huawei.com
Thu Jun 30 19:13:53 PDT 2022
Hi everyone,
Hope I'm not too late for this discussion.
I'm not familiar with ppc so it spent me some time to reproduce this.
But at last I didn't make it.
What I did:
1 checkout to tip/objtool/core
2 apply this patch
3 recover the unreachable() after WARN_ENTRY(..
4 compile on defconfig/allyesconfig
If anyone can point out which file will generate this problem it will be
perfect.
On 2022/6/30 16:05, Naveen N. Rao wrote:
> Christophe Leroy wrote:
>> Hi Sathvika,
>>
>> Adding ARM people as they seem to face the same kind of problem (see
>> https://patchwork.kernel.org/project/linux-kbuild/patch/20220623014917.199563-33-chenzhongjin@huawei.com/)
For my situation, the problem is, if there is an unreachable() used in
the switch default case with nothing else, compiler will generate a NOP
and is still a jump to this NOP branch while it is marked in
.discard.unreachable.
When objtool deal with .discard.unreachable, it will *do nothing* to
this NOP itself, but mark the previous instruction as "dead_end" (see
check.c:ignore_unreachable_insn()). And checking will stop when reach
this dead_end insn.
0x4: jne 0x14 <= jump for switch case
..
0x10: ret <= dead_end
0x14: nop <= real unreachable instructiond
However, actually we have a jump to NOP, which makes this reachable to
this branch, and when this NOP is at end of function, it get a "fall
through" warning.
I changed the unreachable to -EINVAL but it was criticized by the
committer because he thought it is objtool's job to deal with these
special cases.
>>
>> Le 27/06/2022 à 17:35, Sathvika Vasireddy a écrit :
>>>
>>> On 25/06/22 12:16, Christophe Leroy wrote:
>>>>
>>>> Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :
>>>>> objtool is throwing *unannotated intra-function call*
>>>>> warnings with a few instructions that are marked
>>>>> unreachable. Remove unreachable() from WARN_ON()
>>>>> to fix these warnings, as the codegen remains same
>>>>> with and without unreachable() in WARN_ON().
>>>> Did you try the two exemples described in commit 1e688dd2a3d6
>>>> ("powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS()
>>>> with
>>>> asm goto") ?
>>>>
>>>> Without your patch:
>>>>
>>>> 00000640 <test>:
>>>> 640: 81 23 00 84 lwz r9,132(r3)
>>>> 644: 71 29 40 00 andi. r9,r9,16384
>>>> 648: 40 82 00 0c bne 654 <test+0x14>
>>>> 64c: 80 63 00 0c lwz r3,12(r3)
>>>> 650: 4e 80 00 20 blr
>>>> 654: 0f e0 00 00 twui r0,0
>>>>
>>>> 00000658 <test9w>:
>>>> 658: 2c 04 00 00 cmpwi r4,0
>>>> 65c: 41 82 00 0c beq 668 <test9w+0x10>
>>>> 660: 7c 63 23 96 divwu r3,r3,r4
>>>> 664: 4e 80 00 20 blr
>>>> 668: 0f e0 00 00 twui r0,0
>>>> 66c: 38 60 00 00 li r3,0
>>>> 670: 4e 80 00 20 blr
>>>>
>>>>
>>>> With your patch:
>>>>
>>>> 00000640 <test>:
>>>> 640: 81 23 00 84 lwz r9,132(r3)
>>>> 644: 71 29 40 00 andi. r9,r9,16384
>>>> 648: 40 82 00 0c bne 654 <test+0x14>
>>>> 64c: 80 63 00 0c lwz r3,12(r3)
>>>> 650: 4e 80 00 20 blr
>>>> 654: 0f e0 00 00 twui r0,0
>>>> 658: 4b ff ff f4 b 64c <test+0xc> <==
>>>>
>>>> 0000065c <test9w>:
>>>> 65c: 2c 04 00 00 cmpwi r4,0
>>>> 660: 41 82 00 0c beq 66c <test9w+0x10>
>>>> 664: 7c 63 23 96 divwu r3,r3,r4
>>>> 668: 4e 80 00 20 blr
>>>> 66c: 0f e0 00 00 twui r0,0
>>>> 670: 38 60 00 00 li r3,0 <==
>>>> 674: 4e 80 00 20 blr <==
>>>> 678: 38 60 00 00 li r3,0
>>>> 67c: 4e 80 00 20 blr
>>>>
>>> The builtin variant of unreachable (__builtin_unreachable()) works.
>>>
>>> How about using that instead of unreachable() ?
>>>
>>>
>>
>> In fact the problem comes from the macro annotate_unreachable() which
>> is called by unreachable() before calling __build_unreachable().
>>
>> Seems like this macro adds (after the unconditional trap twui) a call
>> to an empty function whose address is listed in section
>> .discard.unreachable
>>
>> 1c78: 00 00 e0 0f twui r0,0
>> 1c7c: 55 e7 ff 4b bl 3d0
>> <qdisc_root_sleeping_lock.part.0>
>>
>>
>> RELOCATION RECORDS FOR [.discard.unreachable]:
>> OFFSET TYPE VALUE
>> 0000000000000000 R_PPC64_REL32 .text+0x00000000000003d0
>>
>> The problem is that that function has size 0:
>>
>> 00000000000003d0 l F .text 0000000000000000
>> qdisc_root_sleeping_lock.part.0
>>
>>
>> And objtool is not prepared for a function with size 0.
>
> annotate_unreachable() seems to have been introduced in commit
> 649ea4d5a624f0 ("objtool: Assume unannotated UD2 instructions are dead
> ends").
>
> Objtool considers 'ud2' instruction to be fatal, so BUG() has
> __builtin_unreachable(), rather than unreachable(). See commit
> bfb1a7c91fb775 ("x86/bug: Merge annotate_reachable() into _BUG_FLAGS()
> asm"). For the same reason, __WARN_FLAGS() is annotated with
> _ASM_REACHABLE so that objtool can differentiate warnings from a BUG().
>
> On powerpc, we use trap variants for both and don't have a special
> instruction for a BUG(). As such, for _WARN_FLAGS(), using
> __builtin_unreachable() suffices to achieve optimal code generation
> from the compiler. Objtool would consider subsequent instructions to
> be reachable. For BUG(), we can continue to use unreachable() so that
> objtool can differentiate these from traps used in warnings.
>
It is right and on arm64 only BUG() has unreachable and there is no
annotation for __WARN_FLAGS(). Objtool works correctly on this. For that
I support that unreachable() annotation shouldn't be with __WARN_FLAGS()
because there should be an accessible branch after WARN() micro. However
in the previous case, it's wired that compiler generates a bl to
unreachable symbol, IIUC it is not a illegal call? (if it is allowed on
ppc then objtool should be tell to recognize this)
It seems that your decoding only care about INSN_CALL for mcount, so
maybe temporarily these control flow checking makes non-sense for you so
the solution could actually be looser.
Anyway, maybe I can help more if I can reproduce that on my own machine.
Best,
Chen
.
More information about the linux-arm-kernel
mailing list