2020-03-25 17:49:38

by Peter Zijlstra

Subject: [PATCH v4 02/13] objtool: Factor out CFI hints

Move the application of CFI hints into it's own function.
No functional changes intended.

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 67 ++++++++++++++++++++++++++++----------------------
1 file changed, 38 insertions(+), 29 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2033,6 +2033,41 @@ static int validate_return(struct symbol
return 0;
}

+static int apply_insn_hint(struct objtool_file *file, struct section *sec,
+ struct symbol *func, struct instruction *insn,
+ struct insn_state *state)
+{
+ if (insn->restore) {
+ struct instruction *save_insn, *i;
+
+ i = insn;
+ save_insn = NULL;
+ sym_for_each_insn_continue_reverse(file, func, i) {
+ if (i->save) {
+ save_insn = i;
+ break;
+ }
+ }
+
+ if (!save_insn) {
+ WARN_FUNC("no corresponding CFI save for CFI restore",
+ sec, insn->offset);
+ return 1;
+ }
+
+ if (!save_insn->visited) {
+ WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
+ sec, insn->offset);
+ return 1;
+ }
+
+ insn->state = save_insn->state;
+ }
+
+ state = insn->state;
+ return 0;
+}
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -2081,35 +2116,9 @@ static int validate_branch(struct objtoo
}

if (insn->hint) {
- if (insn->restore) {
- struct instruction *save_insn, *i;
-
- i = insn;
- save_insn = NULL;
- sym_for_each_insn_continue_reverse(file, func, i) {
- if (i->save) {
- save_insn = i;
- break;
- }
- }
-
- if (!save_insn) {
- WARN_FUNC("no corresponding CFI save for CFI restore",
- sec, insn->offset);
- return 1;
- }
-
- if (!save_insn->visited) {
- WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
- sec, insn->offset);
- return 1;
- }
-
- insn->state = save_insn->state;
- }
-
- state = insn->state;
-
+ ret = apply_insn_hint(file, sec, func, insn, &state);
+ if (ret)
+ return ret;
} else
insn->state = state;

2020-03-25 18:26:53

by Miroslav Benes

Subject: Re: [PATCH v4 02/13] objtool: Factor out CFI hints

On Wed, 25 Mar 2020, Peter Zijlstra wrote:

> Move the application of CFI hints into it's own function.
> No functional changes intended.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Acked-by: Josh Poimboeuf <[email protected]>
> ---
> tools/objtool/check.c | 67 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 38 insertions(+), 29 deletions(-)
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2033,6 +2033,41 @@ static int validate_return(struct symbol
> return 0;
> }
>
> +static int apply_insn_hint(struct objtool_file *file, struct section *sec,
> + struct symbol *func, struct instruction *insn,
> + struct insn_state *state)
> +{
> + if (insn->restore) {
> + struct instruction *save_insn, *i;
> +
> + i = insn;
> + save_insn = NULL;
> + sym_for_each_insn_continue_reverse(file, func, i) {
> + if (i->save) {
> + save_insn = i;
> + break;
> + }
> + }
> +
> + if (!save_insn) {
> + WARN_FUNC("no corresponding CFI save for CFI restore",
> + sec, insn->offset);
> + return 1;
> + }
> +
> + if (!save_insn->visited) {
> + WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> + sec, insn->offset);
> + return 1;
> + }
> +
> + insn->state = save_insn->state;
> + }
> +
> + state = insn->state;

It does not matter, because it will change later again, but there should
be

*state = insn->state;

here, right?

> + return 0;
> +}
> +
> /*
> * Follow the branch starting at the given instruction, and recursively follow
> * any other branches (jumps). Meanwhile, track the frame pointer state at
> @@ -2081,35 +2116,9 @@ static int validate_branch(struct objtoo
> }
>
> if (insn->hint) {
> - if (insn->restore) {
> - struct instruction *save_insn, *i;
> -
> - i = insn;
> - save_insn = NULL;
> - sym_for_each_insn_continue_reverse(file, func, i) {
> - if (i->save) {
> - save_insn = i;
> - break;
> - }
> - }
> -
> - if (!save_insn) {
> - WARN_FUNC("no corresponding CFI save for CFI restore",
> - sec, insn->offset);
> - return 1;
> - }
> -
> - if (!save_insn->visited) {
> - WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> - sec, insn->offset);
> - return 1;
> - }
> -
> - insn->state = save_insn->state;
> - }
> -
> - state = insn->state;
> -
> + ret = apply_insn_hint(file, sec, func, insn, &state);
> + if (ret)
> + return ret;
> } else
> insn->state = state;
>
>
>

2020-03-25 19:41:50

by Peter Zijlstra

Subject: Re: [PATCH v4 02/13] objtool: Factor out CFI hints

On Wed, Mar 25, 2020 at 07:26:06PM +0100, Miroslav Benes wrote:
> On Wed, 25 Mar 2020, Peter Zijlstra wrote:
>
> > Move the application of CFI hints into it's own function.
> > No functional changes intended.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Acked-by: Josh Poimboeuf <[email protected]>
> > ---
> > tools/objtool/check.c | 67 ++++++++++++++++++++++++++++----------------------
> > 1 file changed, 38 insertions(+), 29 deletions(-)
> >
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -2033,6 +2033,41 @@ static int validate_return(struct symbol
> > return 0;
> > }
> >
> > +static int apply_insn_hint(struct objtool_file *file, struct section *sec,
> > + struct symbol *func, struct instruction *insn,
> > + struct insn_state *state)
> > +{
> > + if (insn->restore) {
> > + struct instruction *save_insn, *i;
> > +
> > + i = insn;
> > + save_insn = NULL;
> > + sym_for_each_insn_continue_reverse(file, func, i) {
> > + if (i->save) {
> > + save_insn = i;
> > + break;
> > + }
> > + }
> > +
> > + if (!save_insn) {
> > + WARN_FUNC("no corresponding CFI save for CFI restore",
> > + sec, insn->offset);
> > + return 1;
> > + }
> > +
> > + if (!save_insn->visited) {
> > + WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> > + sec, insn->offset);
> > + return 1;
> > + }
> > +
> > + insn->state = save_insn->state;
> > + }
> > +
> > + state = insn->state;
>
> It does not matter, because it will change later again, but there should
> be
>
> *state = insn->state;
>
> here, right?

Argh, yes. Let me go edit the patches.