Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON()

On Mon, Jul 04, 2022 at 12:44:30PM +0000, Christophe Leroy wrote:

Le 04/07/2022 à 14:05, Peter Zijlstra a écrit :

quoted

On Sat, Jun 25, 2022 at 06:46:54AM +0000, Christophe Leroy wrote:

quoted

Le 24/06/2022 à 20:32, Sathvika Vasireddy a écrit :

quoted

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

Per this construct you should do as x86 does and assume twui terminates
control flow and explicitly annotate the WARN case. That is, given the
fact that BUG as no instructions following it, you can't very well
annotate that.

That exactly the problem I guess. I'm fine with replacing the 
unreachable() by __builtin_unreachable() with our __WARN_FLAGS() and 
BUG() but we will still have a problem with some of the unrachable() 
that are in core parts of the kernel.

Even the ones in arch/powerpc/, they are valid and should remain. The 
point seems that the generic annotate_unreachable() is wrong for powerpc 
as is, and activating CONFIG_OBJTOOL lead to bad code generation.

Right; I'm not against making that depend on yet-another OBJTOOL_$config
thing.

By the way, for which functionnalities of objtool is that analysis 
necessary ? I understand it is not necessary to mcount accounting, so 
maybe the not empty annotate_unreachable() should be limited to those 
those functionnalities ?

For all the things where it needs to follow control flow, so stack
validation, ORC generation, unreachable instruction validation etc..

I'd need to double check code-gen on x86, but the way
__builtin_unreachable() makes code-gen stop dead, I'm not entirely sure
it's right for us either.

quoted

Alternatively, you can teach objtool to look at __bug_table to
distinguish these cases.

Isn't it enough to tell objtool that execution never go past twui, using 
INSN_BUG ?

That should work I suppose.

By the way, for __WARN_FLAGS, we use the __extable for the continuation. 
Is objtools able to follow __extable ?

Yes.