Hi Josh,
i tested the patch on my maschine it works fine.
Best regards
Damian
On Tue, 08. May 22:46, Josh Poimboeuf wrote:
> On Tue, May 08, 2018 at 09:32:41AM -0500, Josh Poimboeuf wrote:
> > On Tue, May 08, 2018 at 04:25:15PM +0200, Greg KH wrote:
> > > Much nicer, thanks for the patch, seems to build things "quieter" now.
> > > For the other warnings, they are "safe" to ignore, right? It's just
> > > objtool checking to see if all is ok, but the result should be fine no
> > > matter what, right?
> >
> > Yeah, from what I can tell, there's nothing catastrophic in the rest of
> > the warnings. Worst case, the ORC unwinder might get confused in a few
> > places.
>
> FWIW, here's the latest version of the patch. This fixes all the
> warnings on my config at least. I'll be sending it to -tip soon if it
> survives 0-day testing.
>
> From: Josh Poimboeuf <jpoim...@redhat.com>
> Subject: [PATCH] objtool: Detect GCC 8 cold subfunctions
>
> GCC 8 moves a lot of unlikely code out of line to "cold" subfunctions in
> .text.unlikely. Properly detect the new subfunctions and treat them as
> extensions of the original functions.
>
> This fixes a bunch of warnings like:
>
> kernel/cgroup/cgroup.o: warning: objtool: parse_cgroup_root_flags()+0x33:
> sibling call from callable instruction with modified stack frame
> kernel/cgroup/cgroup.o: warning: objtool: cgroup_addrm_files()+0x290:
> sibling call from callable instruction with modified stack frame
> kernel/cgroup/cgroup.o: warning: objtool:
> cgroup_apply_control_enable()+0x25b: sibling call from callable instruction
> with modified stack frame
> kernel/cgroup/cgroup.o: warning: objtool: rebind_subsystems()+0x325:
> sibling call from callable instruction with modified stack frame
>
> Reported-by: Arnd Bergmann <a...@arndb.de>
> Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
> ---
> tools/objtool/check.c | 104 +++++++++++++++++++++++++-----------------
> tools/objtool/elf.c | 42 ++++++++++++++++-
> tools/objtool/elf.h | 2 +
> 3 files changed, 104 insertions(+), 44 deletions(-)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 264522d4e4af..ee72958d3464 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -59,6 +59,31 @@ static struct instruction *next_insn_same_sec(struct
> objtool_file *file,
> return next;
> }
>
> +static struct instruction *next_insn_same_func(struct objtool_file *file,
> + struct instruction *insn)
> +{
> + struct instruction *next = list_next_entry(insn, list);
> + struct symbol *func = insn->func;
> +
> + if (!func)
> + return NULL;
> +
> + if (&next->list != &file->insn_list && next->func == func)
> + return next;
> +
> + /* Check if we're already in the subfunction: */
> + if (func == func->cfunc)
> + return NULL;
> +
> + /* Move to the subfunction: */
> + return find_insn(file, func->cfunc->sec, func->cfunc->offset);
> +}
> +
> +#define func_for_each_insn_all(file, func, insn) \
> + for (insn = find_insn(file, func->sec, func->offset); \
> + insn; \
> + insn = next_insn_same_func(file, insn))
> +
> #define func_for_each_insn(file, func, insn) \
> for (insn = find_insn(file, func->sec, func->offset); \
> insn && &insn->list != &file->insn_list && \
> @@ -149,10 +174,14 @@ static int __dead_end_function(struct objtool_file
> *file, struct symbol *func,
> if (!strcmp(func->name, global_noreturns[i]))
> return 1;
>
> - if (!func->sec)
> + if (!func->len)
> return 0;
>
> - func_for_each_insn(file, func, insn) {
> + insn = find_insn(file, func->sec, func->offset);
> + if (!insn->func)
> + return 0;
> +
> + func_for_each_insn_all(file, func, insn) {
> empty = false;
>
> if (insn->type == INSN_RETURN)
> @@ -167,28 +196,17 @@ static int __dead_end_function(struct objtool_file
> *file, struct symbol *func,
> * case, the function's dead-end status depends on whether the target
> * of the sibling call returns.
> */
> - func_for_each_insn(file, func, insn) {
> - if (insn->sec != func->sec ||
> - insn->offset >= func->offset + func->len)
> - break;
> -
> + func_for_each_insn_all(file, func, insn) {
> if (insn->type == INSN_JUMP_UNCONDITIONAL) {
> struct instruction *dest = insn->jump_dest;
> - struct symbol *dest_func;
>
> if (!dest)
> /* sibling call to another file */
> return 0;
>
> - if (dest->sec != func->sec ||
> - dest->offset < func->offset ||
> - dest->offset >= func->offset + func->len) {
> - /* local sibling call */
> - dest_func = find_symbol_by_offset(dest->sec,
> - dest->offset);
> - if (!dest_func)
> - continue;
> + if (dest->func && dest->func->pfunc !=
> insn->func->pfunc) {
>
> + /* local sibling call */
> if (recursion == 5) {
> /*
> * Infinite recursion: two functions
> @@ -199,7 +217,7 @@ static int __dead_end_function(struct objtool_file *file,
> struct symbol *func,
> return 0;
> }
>
> - return __dead_end_function(file, dest_func,
> + return __dead_end_function(file, dest->func,
> recursion + 1);
> }
> }
> @@ -426,7 +444,7 @@ static void add_ignores(struct objtool_file *file)
> if (!ignore_func(file, func))
> continue;
>
> - func_for_each_insn(file, func, insn)
> + func_for_each_insn_all(file, func, insn)
> insn->ignore = true;
> }
> }
> @@ -786,30 +804,28 @@ static int add_special_section_alts(struct objtool_file
> *file)
> return ret;
> }
>
> -static int add_switch_table(struct objtool_file *file, struct symbol *func,
> - struct instruction *insn, struct rela *table,
> - struct rela *next_table)
> +static int add_switch_table(struct objtool_file *file, struct instruction
> *insn,
> + struct rela *table, struct rela *next_table)
> {
> struct rela *rela = table;
> struct instruction *alt_insn;
> struct alternative *alt;
> + struct symbol *pfunc = insn->func->pfunc;
> + unsigned int prev_offset = 0;
>
> list_for_each_entry_from(rela, &file->rodata->rela->rela_list, list) {
> if (rela == next_table)
> break;
>
> - if (rela->sym->sec != insn->sec ||
> - rela->addend <= func->offset ||
> - rela->addend >= func->offset + func->len)
> + if (prev_offset && rela->offset != prev_offset + sizeof(long))
> break;
>
> - alt_insn = find_insn(file, insn->sec, rela->addend);
> - if (!alt_insn) {
> - WARN("%s: can't find instruction at %s+0x%x",
> - file->rodata->rela->name, insn->sec->name,
> - rela->addend);
> - return -1;
> - }
> + alt_insn = find_insn(file, rela->sym->sec, rela->addend);
> + if (!alt_insn)
> + break;
> +
> + if (alt_insn->func->pfunc != pfunc)
> + break;
>
> alt = malloc(sizeof(*alt));
> if (!alt) {
> @@ -819,6 +835,13 @@ static int add_switch_table(struct objtool_file *file,
> struct symbol *func,
>
> alt->insn = alt_insn;
> list_add_tail(&alt->list, &insn->alts);
> + prev_offset = rela->offset;
> + }
> +
> + if (!prev_offset) {
> + WARN_FUNC("can't find switch jump table",
> + insn->sec, insn->offset);
> + return -1;
> }
>
> return 0;
> @@ -947,7 +970,7 @@ static int add_func_switch_tables(struct objtool_file
> *file,
> struct rela *rela, *prev_rela = NULL;
> int ret;
>
> - func_for_each_insn(file, func, insn) {
> + func_for_each_insn_all(file, func, insn) {
> if (!last)
> last = insn;
>
> @@ -978,8 +1001,7 @@ static int add_func_switch_tables(struct objtool_file
> *file,
> * the beginning of another switch table in the same function.
> */
> if (prev_jump) {
> - ret = add_switch_table(file, func, prev_jump, prev_rela,
> - rela);
> + ret = add_switch_table(file, prev_jump, prev_rela,
> rela);
> if (ret)
> return ret;
> }
> @@ -989,7 +1011,7 @@ static int add_func_switch_tables(struct objtool_file
> *file,
> }
>
> if (prev_jump) {
> - ret = add_switch_table(file, func, prev_jump, prev_rela, NULL);
> + ret = add_switch_table(file, prev_jump, prev_rela, NULL);
> if (ret)
> return ret;
> }
> @@ -1753,15 +1775,13 @@ static int validate_branch(struct objtool_file *file,
> struct instruction *first,
> while (1) {
> next_insn = next_insn_same_sec(file, insn);
>
> -
> - if (file->c_file && func && insn->func && func != insn->func) {
> + if (file->c_file && func && insn->func && func !=
> insn->func->pfunc) {
> WARN("%s() falls through to next function %s()",
> func->name, insn->func->name);
> return 1;
> }
>
> - if (insn->func)
> - func = insn->func;
> + func = insn->func ? insn->func->pfunc : NULL;
>
> if (func && insn->ignore) {
> WARN_FUNC("BUG: why am I validating an ignored
> function?",
> @@ -1782,7 +1802,7 @@ static int validate_branch(struct objtool_file *file,
> struct instruction *first,
>
> i = insn;
> save_insn = NULL;
> - func_for_each_insn_continue_reverse(file, func,
> i) {
> + func_for_each_insn_continue_reverse(file,
> insn->func, i) {
> if (i->save) {
> save_insn = i;
> break;
> @@ -1869,7 +1889,7 @@ static int validate_branch(struct objtool_file *file,
> struct instruction *first,
> case INSN_JUMP_UNCONDITIONAL:
> if (insn->jump_dest &&
> (!func || !insn->jump_dest->func ||
> - func == insn->jump_dest->func)) {
> + insn->jump_dest->func->pfunc == func)) {
> ret = validate_branch(file, insn->jump_dest,
> state);
> if (ret)
> @@ -2064,7 +2084,7 @@ static int validate_functions(struct objtool_file *file)
>
> for_each_sec(file, sec) {
> list_for_each_entry(func, &sec->symbol_list, list) {
> - if (func->type != STT_FUNC)
> + if (func->type != STT_FUNC || func->pfunc != func)
> continue;
>
> insn = find_insn(file, sec, func->offset);
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index c1c338661699..4e60e105583e 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -79,6 +79,19 @@ struct symbol *find_symbol_by_offset(struct section *sec,
> unsigned long offset)
> return NULL;
> }
>
> +struct symbol *find_symbol_by_name(struct elf *elf, const char *name)
> +{
> + struct section *sec;
> + struct symbol *sym;
> +
> + list_for_each_entry(sec, &elf->sections, list)
> + list_for_each_entry(sym, &sec->symbol_list, list)
> + if (!strcmp(sym->name, name))
> + return sym;
> +
> + return NULL;
> +}
> +
> struct symbol *find_symbol_containing(struct section *sec, unsigned long
> offset)
> {
> struct symbol *sym;
> @@ -203,10 +216,11 @@ static int read_sections(struct elf *elf)
>
> static int read_symbols(struct elf *elf)
> {
> - struct section *symtab;
> - struct symbol *sym;
> + struct section *symtab, *sec;
> + struct symbol *sym, *pfunc;
> struct list_head *entry, *tmp;
> int symbols_nr, i;
> + char *coldstr;
>
> symtab = find_section_by_name(elf, ".symtab");
> if (!symtab) {
> @@ -281,6 +295,30 @@ static int read_symbols(struct elf *elf)
> hash_add(sym->sec->symbol_hash, &sym->hash, sym->idx);
> }
>
> + /* Create parent/child links for any cold subfunctions */
> + list_for_each_entry(sec, &elf->sections, list) {
> + list_for_each_entry(sym, &sec->symbol_list, list) {
> + if (sym->type != STT_FUNC)
> + continue;
> + sym->pfunc = sym->cfunc = sym;
> + coldstr = strstr(sym->name, ".cold.");
> + if (coldstr) {
> + coldstr[0] = '\0';
> + pfunc = find_symbol_by_name(elf, sym->name);
> + coldstr[0] = '.';
> +
> + if (!pfunc) {
> + WARN("%s(): can't find parent function",
> + sym->name);
> + goto err;
> + }
> +
> + sym->pfunc = pfunc;
> + pfunc->cfunc = sym;
> + }
> + }
> + }
> +
> return 0;
>
> err:
> diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
> index d86e2ff14466..de5cd2ddded9 100644
> --- a/tools/objtool/elf.h
> +++ b/tools/objtool/elf.h
> @@ -61,6 +61,7 @@ struct symbol {
> unsigned char bind, type;
> unsigned long offset;
> unsigned int len;
> + struct symbol *pfunc, *cfunc;
> };
>
> struct rela {
> @@ -86,6 +87,7 @@ struct elf {
> struct elf *elf_open(const char *name, int flags);
> struct section *find_section_by_name(struct elf *elf, const char *name);
> struct symbol *find_symbol_by_offset(struct section *sec, unsigned long
> offset);
> +struct symbol *find_symbol_by_name(struct elf *elf, const char *name);
> struct symbol *find_symbol_containing(struct section *sec, unsigned long
> offset);
> struct rela *find_rela_by_dest(struct section *sec, unsigned long offset);
> struct rela *find_rela_by_dest_range(struct section *sec, unsigned long
> offset,
> --
> 2.17.0
>
