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