Re: [PATCH 3/7] objtool: Add support for intra-function calls

[Julien Thierry]

On 4/2/20 2:24 PM, Alexandre Chartre wrote:
> On 4/2/20 2:53 PM, Julien Thierry wrote:
> > Hi Alexandre,
> >
> > I ran into the limitation of intra-function call for the arm64
> > support but didn't take the time to make a clean patch to support
> > them properly.
> >
> > Nice to see you've gone through that work :) .
> >
> > On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> > > Change objtool to support intra-function calls. An intra-function call
> > > is represented in objtool as a push onto the stack (of the return
> >
> > I have to disagree a bit with that. The push onto the stack is true
> > on x86, but other architectures might not have that (arm/arm64 have a
> > link register that gets set by "bl" instructions and do not modify
> > the stack).
>
> Correct, this is x86 specific.
>
> > > address), and a jump to the destination address. That way the stack
> > > information is correctly updated and the call flow is still accurate.
> > >
> > > Signed-off-by: Alexandre Chartre <alexandre.chartre@xxxxxxxxxx>
> > > ---
> > >   tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
> > >   tools/objtool/check.h |  1 +
> > >   2 files changed, 73 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > index 214809ac2776..0cec91291d46 100644
> > > --- a/tools/objtool/check.c
> > > +++ b/tools/objtool/check.c
> > > @@ -657,6 +657,18 @@ static int add_call_destinations(struct 
> > > objtool_file *file)
> > >           if (insn->type != INSN_CALL)
> > >               continue;
> > > +        if (insn->intra_function_call) {
> > > +            dest_off = insn->offset + insn->len + insn->immediate;
> > > +            insn->jump_dest = find_insn(file, insn->sec, dest_off);
> > > +            if (insn->jump_dest)
> > > +                continue;
> > > +
> > > +            WARN_FUNC("can't find call dest at %s+0x%lx",
> > > +                  insn->sec, insn->offset,
> > > +                  insn->sec->name, dest_off);
> > > +            return -1;
> > > +        }
> > > +
> > >           rela = find_rela_by_dest_range(insn->sec, insn->offset,
> > >                              insn->len);
> > >           if (!rela) {
> > > @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct 
> > > objtool_file *file)
> > >       return 0;
> > >   }
> > > +static int read_intra_function_call(struct objtool_file *file)
> > > +{
> > > +    struct section *sec;
> > > +    struct instruction *insn;
> > > +    struct rela *rela;
> > > +
> > > +    sec = find_section_by_name(file->elf,
> > > +                   ".rela.discard.intra_function_call");
> >
> > I'm wondering, do we really need to annotate the intra_function_call
> > and group the in a section?
> >
> > Would it be a problem to consider all (static) call instructions with
> > a destination that is not the start offset of a symbol to be an
> > intra-function call (and set insn->intra_function_call and
> > insn->jump_dest accordingly)?
>
> Correct, we could automatically detect intra-function calls instead of
> having to annotate them. However, I choose to annotate them because I don't
> think that's not an expected construct in a "normal" code flow (at least
> on x86). So objtool would still issue a warning on intra-function calls
> by default, and you can annotate them to indicate if they are expected.
>
> If intra-function calls are frequent on arm then I can add an option to
> objtool so it automatically detects them. This way, we won't use the option
> on x86 and we have to annotate intra-function call on x86, and you can
> use it on arm to automatically detect intra-function calls.

That makes sense. Maybe we can just allow them in !file->c_file, I don't 
think gcc generates such call on arm64, so I think we'd only have that 
in assembly.

If people prefer to keep the annotation, would you mind having a 
"ANNOTATE_INTRA_FUNCTION_CALL" macro in include/linux/frame.h to add the 
label and the reference to the right section?
This way it could be reused for other archs.

> > Other than that the logic would stay the same.
> >
> > > +    if (!sec)
> > > +        return 0;
> > > +
> > > +    list_for_each_entry(rela, &sec->rela_list, list) {
> > > +        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 call",
> > > +                  insn->sec, insn->offset);
> >
> > Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
> >      "unsupported instruction for intra-function call " ?
>
> Right, I will change that: "intra_function_call not a direct call"
>
> > > +            return -1;
> > > +        }
> > > +
> > > +        insn->intra_function_call = true;
> > > +        /*
> > > +         * For the impact on the stack, make an intra-function
> > > +         * call behaves like a push of an immediate value (the
> > > +         * return address).
> > > +         */
> > > +        insn->stack_op.src.type = OP_SRC_CONST;
> > > +        insn->stack_op.dest.type = OP_DEST_PUSH;
> >
> > As commented above, this should be arch dependent.
>
> I will add a arch dependent call. I will also do that for the return
> trampoline call case (patch 4).

Thank you!

--
Julien Thierry