Messages in this thread | | | Date | Thu, 30 Jun 2022 16:07:47 +0530 | From | "Naveen N. Rao" <> | Subject | Re: [RFC PATCH v3 11/12] powerpc: Remove unreachable() from WARN_ON() |
| |
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
| |