Re: [PATCH] objtool: Don't add empty symbols to the rbtree

[Peter Zijlstra]

On Wed, Jan 06, 2021 at 09:33:20PM -0600, Josh Poimboeuf wrote:
> Building with the Clang assembler shows the following warning:
> 
>   arch/x86/kernel/ftrace_64.o: warning: objtool: missing symbol for insn at offset 0x16
> 
> The Clang assembler strips section symbols.  That ends up giving
> objtool's find_func_containing() much more test coverage than normal.
> Turns out, find_func_containing() doesn't work so well for overlapping
> symbols:
> 
>      2: 000000000000000e     0 NOTYPE  LOCAL  DEFAULT    2 fgraph_trace
>      3: 000000000000000f     0 NOTYPE  LOCAL  DEFAULT    2 trace
>      4: 0000000000000000   165 FUNC    GLOBAL DEFAULT    2 __fentry__
>      5: 000000000000000e     0 NOTYPE  GLOBAL DEFAULT    2 ftrace_stub
> 
> The zero-length NOTYPE symbols are inside __fentry__(), confusing the
> rbtree search for any __fentry__() offset coming after a NOTYPE.
> 
> Try to avoid this problem by not adding zero-length symbols to the
> rbtree.  They're rare and aren't needed in the rbtree anyway.
> 
> One caveat, this actually might not end up being the right fix.
> Non-empty overlapping symbols, if they exist, could have the same
> problem.  But that would need bigger changes, let's see if we can get
> away with the easy fix for now.

Right, overlapping things needs a different data structure, could be
done though, but like you say, moar work.

> Reported-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>

Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>

> ---
>  tools/objtool/elf.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index be89c741ba9a..ccee8fc331f0 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -448,6 +448,13 @@ static int read_symbols(struct elf *elf)
>  		list_add(&sym->list, entry);
>  		elf_hash_add(elf->symbol_hash, &sym->hash, sym->idx);
>  		elf_hash_add(elf->symbol_name_hash, &sym->name_hash, str_hash(sym->name));
> +
> +		/*
> +		 * Don't store empty STT_NOTYPE symbols in the rbtree.  They
> +		 * can exist within a real function, confusing the sorting.
> +		 */
> +		if (!sym->len)
> +			rb_erase(&sym->node, &sym->sec->symbol_tree);
>  	}
>  
>  	if (stats)
> -- 
> 2.29.2
>