Re: [PATCH 4/8] objtool: Add support for intra-function calls

From: Peter Zijlstra
Date: Thu Apr 23 2020 - 11:22:53 EST


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.