| Message ID | 20220624183238.388144-12-sv@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | objtool: Enable and implement --mcount option on powerpc | expand |
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 Christophe > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > arch/powerpc/include/asm/bug.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h > index ecbae1832de3..df6c11e008b9 100644 > --- a/arch/powerpc/include/asm/bug.h > +++ b/arch/powerpc/include/asm/bug.h > @@ -97,7 +97,6 @@ > __label__ __label_warn_on; \ > \ > WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ > - unreachable(); \ > \ > __label_warn_on: \ > break; \
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, and the codegen remains the same. How about using that instead of unreachable() ? - Sathvika
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() ? - Sathvika
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() ? > That seems odd. Look at linux/compiler.h It seems like unreachable() exists to help objtool. Christophe
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
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
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.
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
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
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
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 .
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
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
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.
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?
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.
On Sat, Jun 25, 2022 at 06:46:54AM +0000, 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 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. Alternatively, you can teach objtool to look at __bug_table to distinguish these cases.
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().
Le 04/07/2022 à 14:05, Peter Zijlstra a écrit : > On Sat, Jun 25, 2022 at 06:46:54AM +0000, 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 > > 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. 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 ? > > 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 ? By the way, for __WARN_FLAGS, we use the __extable for the continuation. Is objtools able to follow __extable ? Christophe
On Mon, Jul 04, 2022 at 12:44:30PM +0000, Christophe Leroy wrote: > > > Le 04/07/2022 à 14:05, Peter Zijlstra a écrit : > > On Sat, Jun 25, 2022 at 06:46:54AM +0000, 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 > > > > 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. > > 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.
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
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index ecbae1832de3..df6c11e008b9 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -97,7 +97,6 @@ __label__ __label_warn_on; \ \ WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \ - unreachable(); \ \ __label_warn_on: \ break; \
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(). Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> --- arch/powerpc/include/asm/bug.h | 1 - 1 file changed, 1 deletion(-)