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

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

[Christophe Leroy]

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.


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);
  }

---

I also had objtool running for ever on 
arch/powerpc/sysdev/xics/icp-hv.o, which I fixed with the below hack:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 51b6dcec8d6a..ef2303ad6381 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -529,7 +529,7 @@ static struct instruction *find_last_insn(struct 
objtool_file *file,
  	unsigned int offset;
  	unsigned int end = (sec->sh.sh_size > 10) ? sec->sh.sh_size - 10 : 0;

-	for (offset = sec->sh.sh_size - 1; offset >= end && !insn; offset--)
+	for (offset = sec->sh.sh_size - 1; offset && offset >= end && !insn; 
offset--)
  		insn = find_insn(file, sec, offset);

  	return insn;
---

Now I only have the following two warnings:

arch/powerpc/sysdev/xics/icp-hv.o: warning: objtool: can't find 
unreachable insn at .text.unlikely+0x0
drivers/crypto/vmx/aesp8-ppc.o: warning: objtool: 
aes_p8_set_encrypt_key+0x44: unannotated intra-function call

The first one is linked to the infinite loop I hacked. So I now have to 
understand what the problem really is.

The second one is an assembly file aesp8-ppc.S which lacks the changes 
you did in other assembly files in patch 12.

Christophe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Naveen N. Rao]

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.

> 
> 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?


- Naveen


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christophe Leroy]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christophe Leroy]

Le 30/06/2022 à 11:58, Christophe Leroy a écrit :
> 
> 
> 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 get a nice result with the following changes (on top of Sathvika's 
series):

diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index df6c11e008b9..73f5650f98df 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -89,7 +89,7 @@

  #define BUG() do {						\
  	BUG_ENTRY("twi 31, 0, 0", 0);				\
-	unreachable();						\
+	__builtin_unreachable();				\
  } while (0)
  #define HAVE_ARCH_BUG

@@ -97,6 +97,7 @@
  	__label__ __label_warn_on;				\
  								\
  	WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
+	__builtin_unreachable();				\
  								\
  __label_warn_on:						\
  	break;							\
diff --git a/tools/objtool/arch/powerpc/decode.c 
b/tools/objtool/arch/powerpc/decode.c
index 06fc0206bf8e..9a0303304923 100644
--- a/tools/objtool/arch/powerpc/decode.c
+++ b/tools/objtool/arch/powerpc/decode.c
@@ -68,6 +68,8 @@ int arch_decode_instruction(struct objtool_file *file, 
const struct section *sec
  		}
  		break;
  	}
+	if (insn == 0x0fe00000) /* twui */
+		*type = INSN_BUG;

  	return 0;
  }
---

Christophe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Naveen N. Rao]

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

Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
BUG(), there is no need for an annotation since objtool assumes that 
'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
used, an explicit annotate_reachable() is needed. That's _reachable_, to 
indicate that the control flow can continue with the next instruction.

On powerpc, we should (eventually) classify all trap variants as 
INSN_TRAP. Even in the absence of that classification today, objtool 
assumes that control flow continues with the next instruction. With your 
work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
being generated, I think it is appropriate to just use 
__builtin_unreachable() and to not use the annotation.

In any case, we are only hitting this since gcc is generating a 'bl' due 
to that annotation. We are not yet enabling full objtool validation on 
powerpc, so I think we can revisit this at that point.

> 
>> 
>>>
>>> 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.

Yes, see my explanation above. Since no 'bl' is emitted with the 
builtin, objtool won't complain, especially for mcount.

> 
> 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.

Indeed. Not really sure. annotate_unreachable() wants to take the 
address of the instruction after the trap. But, in reality, due to use 
of asm goto for __WARN_FLAGS, no instructions would be generated. I 
wonder if that combination causes such code to be emitted.


- Naveen


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Segher Boessenkool]

On Thu, Jun 30, 2022 at 04:07:47PM +0530, Naveen N. Rao wrote:
> Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
> BUG(), there is no need for an annotation since objtool assumes that 
> 'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
> used, an explicit annotate_reachable() is needed. That's _reachable_, to 
> indicate that the control flow can continue with the next instruction.
> 
> On powerpc, we should (eventually) classify all trap variants as 
> INSN_TRAP. Even in the absence of that classification today, objtool 
> assumes that control flow continues with the next instruction. With your 
> work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
> being generated, I think it is appropriate to just use 
> __builtin_unreachable() and to not use the annotation.
> 
> In any case, we are only hitting this since gcc is generating a 'bl' due 
> to that annotation. We are not yet enabling full objtool validation on 
> powerpc, so I think we can revisit this at that point.

See also <https://gcc.gnu.org/PR99299> that asks for a __builtin_trap()
variant that does not terminate control flow ("that is recoverable").


Segher

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peter Zijlstra]

On Thu, Jun 30, 2022 at 10:58:11AM -0500, Segher Boessenkool wrote:
> On Thu, Jun 30, 2022 at 04:07:47PM +0530, Naveen N. Rao wrote:
> > Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 
> > BUG(), there is no need for an annotation since objtool assumes that 
> > 'ud2' terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is 
> > used, an explicit annotate_reachable() is needed. That's _reachable_, to 
> > indicate that the control flow can continue with the next instruction.
> > 
> > On powerpc, we should (eventually) classify all trap variants as 
> > INSN_TRAP. Even in the absence of that classification today, objtool 
> > assumes that control flow continues with the next instruction. With your 
> > work to utilize asm goto for __WARN_FLAGS(), with no extra instructions 
> > being generated, I think it is appropriate to just use 
> > __builtin_unreachable() and to not use the annotation.
> > 
> > In any case, we are only hitting this since gcc is generating a 'bl' due 
> > to that annotation. We are not yet enabling full objtool validation on 
> > powerpc, so I think we can revisit this at that point.
> 
> See also <https://gcc.gnu.org/PR99299> that asks for a __builtin_trap()
> variant that does not terminate control flow ("that is recoverable").

Re comment 9 there, x86 must assume ud2 will abort. If the compiler were
to emit ud2 for non-abort purposes then it needs to somehow annotate
this so that both objtool and the kernel can determine this.

That said; the whole annotate_reachable() thing in our WARN is
superfluous, we should read __bug_table instead. That said, we still
need _ASM_REACHABLE to handle a few noreturn cases, so there is no real
pressing reason to go clean that up. (if only the compiler would tell us
about noreturn)

ARM has an immediate in their break instruction and varies the desired
behaviour depending on the immediate.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peter Zijlstra]

On Thu, Jun 30, 2022 at 04:07:47PM +0530, Naveen N. Rao wrote:

> Objtool classifies 'ud2' as INSN_BUG, and 'int3' as INSN_TRAP. In x86 BUG(),

Yes, ud2 is the traditional 'kill' instruction and a number of emulators
treat it as such, however it also being the shortest encoding (2 bytes)
for #UD Linux has opted to (ab)use it to implement WARN/BUG.

As such interpretation of 'ud2' needs to assume control flow stops
(compiler will also emit ud2 in a number of cases with that intent).
However, if it's used as WARN we then need to annotate the thing to not
be terminal.

> there is no need for an annotation since objtool assumes that 'ud2'
> terminates control flow. But, for __WARN_FLAGS(), since 'ud2' is used, an
> explicit annotate_reachable() is needed. That's _reachable_, to indicate
> that the control flow can continue with the next instruction.
> 
> On powerpc, we should (eventually) classify all trap variants as INSN_TRAP.

Careful.. INSN_TRAP is mostly used for purposes of speculation stop and
padding. That is, INSN_TRAP does indeed not affect control flow, but the
way objtool treats it might not be quite what you want.

Specifically, straight-line-speculation checks want INT3 after indirect
control transfers (indirect jump and return -- indirect call is
'difficult'); these locations are architecturally not executed and as
such placing a random trap instruction there is 'harmless'. Of course,
were the branch predictor to go wobbly and attempt to execute it, the
fact that it's a trap will stop speculation dead.

Additionally, int3, being a single byte instruction, is also used to
fill dead code space, any #BP trap on it will not have a descriptor and
mostly cause the kernel to go splat.

Per the last usage, validate_reachable_instructions() will ignore it.
I'm not sure you want to always ignore all your (unreachable) trap
instructions.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Chen Zhongjin]

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

.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sathvika Vasireddy]

Hi Chen,

Thanks for pitching in and providing your inputs :-)

On 01/07/22 07:43, Chen Zhongjin wrote:
> 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.

To reproduce this problem, you may want to apply this patch series on 
top of objtool/core branch of the tip tree, and compile on 
ppc64le_defconfig.

There are a couple of C files that are generating these warnings. One 
such file is: arch/powerpc/kvm/../../../virt/kvm/kvm_main.o which gives
*arch/powerpc/kvm/../../../virt/kvm/kvm_main.o: warning: objtool: 
kvm_mmu_notifier_release+0x6c: unannotated intra-function call* warning.

With unreachable() in __WARN_FLAGS(), we get unannotated intra-function 
call warnings, but without unreachable() like in patch 11/12 or with 
just the builtin variant of unreachable (__builtin_unreachable()) 
instead of unreachable(), we do not get those warnings.


- Sathvika


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christophe Leroy]

Hi Segher, your help might be welcome,

Le 01/07/2022 à 08:56, Sathvika Vasireddy a écrit :
> Hi Chen,
> 
> Thanks for pitching in and providing your inputs :-)
> 
> On 01/07/22 07:43, Chen Zhongjin wrote:
>> 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.
> 
> To reproduce this problem, you may want to apply this patch series on 
> top of objtool/core branch of the tip tree, and compile on 
> ppc64le_defconfig.
> 
> There are a couple of C files that are generating these warnings. One 
> such file is: arch/powerpc/kvm/../../../virt/kvm/kvm_main.o which gives
> *arch/powerpc/kvm/../../../virt/kvm/kvm_main.o: warning: objtool: 
> kvm_mmu_notifier_release+0x6c: unannotated intra-function call* warning.
> 
> With unreachable() in __WARN_FLAGS(), we get unannotated intra-function 
> call warnings, but without unreachable() like in patch 11/12 or with 
> just the builtin variant of unreachable (__builtin_unreachable()) 
> instead of unreachable(), we do not get those warnings.
> 


I made a simple test aside our issue to see if I get something similar 
to ARM. I don't if it is the same at the end, but it looks odd anyway:

int test(int x)
{
	switch(x) {
	case 0 : return 3;
	case 1 : return 5;
	default : unreachable();
	}
}

0000000000001c80 <test>:
     1c80:       a6 02 08 7c     mflr    r0
     1c84:       01 00 00 48     bl      1c84 <test+0x4>
                         1c84: R_PPC64_REL24     _mcount
     1c88:       00 00 23 2c     cmpdi   r3,0
     1c8c:       14 00 82 41     beq     1ca0 <test+0x20>
     1c90:       01 00 03 2c     cmpwi   r3,1
     1c94:       05 00 60 38     li      r3,5
     1c98:       20 00 82 4d     beqlr
     1c9c:       00 00 42 60     ori     r2,r2,0
     1ca0:       03 00 60 38     li      r3,3
     1ca4:       20 00 80 4e     blr

So it looks like it takes no real benefit from the unreachable marking.

Now with __builtin_unreachable() instead of unreachable() :

int test(int x)
{
	switch(x) {
	case 0 : return 3;
	case 1 : return 5;
	default : __builtin_unreachable();
	}
}

0000000000001c80 <test>:
     1c80:       a6 02 08 7c     mflr    r0
     1c84:       01 00 00 48     bl      1c84 <test+0x4>
                         1c84: R_PPC64_REL24     _mcount
     1c88:       00 00 63 20     subfic  r3,r3,0
     1c8c:       10 19 63 7c     subfe   r3,r3,r3
     1c90:       bc 07 63 54     rlwinm  r3,r3,0,30,30
     1c94:       03 00 63 38     addi    r3,r3,3
     1c98:       20 00 80 4e     blr


Here the generated code takes advantage of the unreachable marking and 
results in a branchless code.


Christophe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peter Zijlstra]

On Wed, Jun 29, 2022 at 06:30:23PM +0000, Christophe Leroy wrote:


> The problem is that that function has size 0:
> 
> 00000000000003d0 l     F .text	0000000000000000 
> qdisc_root_sleeping_lock.part.0

I'm somewhat confused; how is an empty STT_FUNC a valid construct on
Power?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christophe Leroy]

Le 04/07/2022 à 13:45, Peter Zijlstra a écrit :
> On Wed, Jun 29, 2022 at 06:30:23PM +0000, Christophe Leroy wrote:
> 
> 
>> The problem is that that function has size 0:
>>
>> 00000000000003d0 l     F .text	0000000000000000
>> qdisc_root_sleeping_lock.part.0
> 
> I'm somewhat confused; how is an empty STT_FUNC a valid construct on
> Power?

So am I. It is likely not a valid construct, but that's what GCC seems 
to generate when you call annotate_unreachable().
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Segher Boessenkool]

On Mon, Jul 04, 2022 at 12:34:08PM +0000, Christophe Leroy wrote:
> Le 04/07/2022 à 13:45, Peter Zijlstra a écrit :
> > I'm somewhat confused; how is an empty STT_FUNC a valid construct on
> > Power?
> 
> So am I. It is likely not a valid construct, but that's what GCC seems 
> to generate when you call annotate_unreachable().

It is a valid construct on (almost) all targets.  If the user chooses to
have executable code terminate in limbo, that is what the compiler will
do (and this can result in a code symbol with size 0).  Compare this to
data symbols with no size, the situation is quite similar.


Segher

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel