Messages in this thread | | | Date | Thu, 23 Apr 2020 17:22:43 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 4/8] objtool: Add support for intra-function calls |
| |
On Thu, Apr 23, 2020 at 04:34:21PM +0200, Miroslav Benes wrote: > > /* > > * Find the destination instructions for all calls. > > */ > > @@ -715,10 +725,7 @@ static int add_call_destinations(struct > > continue; > > > > if (!insn->call_dest) { > > - WARN_FUNC("unsupported intra-function call", > > - insn->sec, insn->offset); > > - if (retpoline) > > - WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE."); > > + WARN_FUNC("intra-function call", insn->sec, insn->offset); > > "unsupported intra-function call"?
Well, I think the thinking was that intra-function calls are actually supported, 'unannotated' perhaps ?
> > return -1; > > } > > > > @@ -741,6 +748,12 @@ static int add_call_destinations(struct > > } > > } else > > insn->call_dest = rela->sym; > > + > > + /* > > + * Whatever stack impact regular CALLs have, should be > > + * undone by the RETURN of the called function. > > * Annotated intra-function CALLs are treated as JMPs with a stack_op. > * See read_intra_function_calls(). > > would make it a bit clearer.
That doesn't work for me; we want to explain why it is OK to delete stack_ops for regular CALLs. The reason this is OK, is because they're matched by RETURN.
> > + */ > > + remove_insn_ops(insn); > > } > > > > return 0; > > @@ -1416,6 +1429,57 @@ static int read_instr_hints(struct objto > > return 0; > > } > > > > +static int read_intra_function_calls(struct objtool_file *file) > > +{ > > + struct instruction *insn; > > + struct section *sec; > > + struct rela *rela; > > + > > + sec = find_section_by_name(file->elf, ".rela.discard.intra_function_calls"); > > + if (!sec) > > + return 0; > > + > > + list_for_each_entry(rela, &sec->rela_list, list) { > > + unsigned long dest_off; > > + > > + if (rela->sym->type != STT_SECTION) { > > + WARN("unexpected relocation symbol type in %s", > > + sec->name); > > + return -1; > > + } > > + > > + insn = find_insn(file, rela->sym->sec, rela->addend); > > + if (!insn) { > > + WARN("bad .discard.intra_function_call entry"); > > + return -1; > > + } > > + > > + if (insn->type != INSN_CALL) { > > + WARN_FUNC("intra_function_call not a direct call", > > + insn->sec, insn->offset); > > + return -1; > > + } > > + > > + /* > > + * Treat intra-function CALLs as JMPs, but with a stack_op. > > + * Also see how setup_call_dest() strips stack_ops from normal > > + * CALLs. > > /* > * Treat annotated intra-function CALLs as JMPs, but with a stack_op. > * Also see how add_call_destinations() strips stack_ops from normal > * CALLs. > */ > > ? (note added "annotated" and s/setup_call_dest/add_call_destinations/)
Unannotated intra-function calls are not allowed, so I don't see a reason to make that distinction, but sure.
> > + */ > > + insn->type = INSN_JUMP_UNCONDITIONAL; > > [...] > > > @@ -2245,6 +2313,9 @@ static int validate_branch(struct objtoo > > return 0; > > } > > > > + if (handle_insn_ops(insn, &state)) > > + return 1; > > + > > switch (insn->type) { > > > > case INSN_RETURN: > > @@ -2304,9 +2375,6 @@ static int validate_branch(struct objtoo > > break; > > > > case INSN_EXCEPTION_RETURN: > > - if (handle_insn_ops(insn, &state)) > > - return 1; > > - > > /* > > * This handles x86's sync_core() case, where we use an > > * IRET to self. All 'normal' IRET instructions are in > > @@ -2326,8 +2394,6 @@ static int validate_branch(struct objtoo > > return 0; > > > > case INSN_STACK: > > - if (handle_insn_ops(insn, &state)) > > - return 1; > > break; > > So we could get rid of INSN_STACK now as Julien proposed, couldn't we? If > I am not missing something. handle_insn_ops() is called unconditionally > here for all insn types and you remove stack_ops when unneeded.
Yes, INSN_STACK can now go away in favour of NOPs with stack_ops. Separate patch though.
> We could also go ahead with Julien's proposal to remove > INSN_EXCEPTION_RETURN hack and move it to tools/objtool/arch/x86/decode.c.
I don't immediately see how; we don't have a symbol there.
| |