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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 30/06/2022 à 10:05, Naveen N. Rao a écrit :
> 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/) 
>>
>>
>> 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.

Not sure I understand what you mean.

__WARN_FLAGS() and BUG() both use 'twui' which is unconditionnal trap, 
as such both are the same.

On the other side, WARN_ON() and BUG_ON() use tlbnei which is a 
conditionnel trap.

> 
>>
>> The following changes to objtool seem to fix the problem, most warning 
>> are gone with that change.
>>
>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>> index 63218f5799c2..37c0a268b7ea 100644
>> --- a/tools/objtool/elf.c
>> +++ b/tools/objtool/elf.c
>> @@ -77,6 +77,8 @@ static int symbol_by_offset(const void *key, const 
>> struct rb_node *node)
>>
>>       if (*o < s->offset)
>>           return -1;
>> +    if (*o == s->offset && !s->len)
>> +        return 0;
>>       if (*o >= s->offset + s->len)
>>           return 1;
>>
>> @@ -400,7 +402,7 @@ static void elf_add_symbol(struct elf *elf, struct 
>> symbol *sym)
>>        * Don't store empty STT_NOTYPE symbols in the rbtree.  They
>>        * can exist within a function, confusing the sorting.
>>        */
>> -    if (!sym->len)
>> +    if (sym->type == STT_NOTYPE && !sym->len)
>>           rb_erase(&sym->node, &sym->sec->symbol_tree);
>>   }
> 
> Is there a reason to do this, rather than change __WARN_FLAGS() to use 
> __builtin_unreachable()? Or, are you seeing an issue with unreachable() 
> elsewhere in the kernel?
> 

At the moment I'm trying to understand what the issue is, and explore 
possible fixes. I guess if we tell objtool that after 'twui' subsequent 
instructions are unreachable, then __builtin_unreachable() is enough.

I think we should also understand why annotate_unreachable() gives us a 
so bad result and see if it can be changed to something cleaner than a 
'bl' to an empty function that has no instructions.