Re: [PATCH 6/9] objtool: Add elf_create_undef_symbol()

[Miroslav Benes <mbenes@suse.cz>]

[ also correcting my e-mail address ]

On Fri, 12 Mar 2021, Peter Zijlstra wrote:

Just a remark regarding SHN_XINDEX...

> +static bool elf_symbol_add(struct elf *elf, struct symbol *sym, Elf32_Word shndx)
> +{
> +	struct list_head *entry;
> +	struct rb_node *pnode;
> +
> +	sym->type = GELF_ST_TYPE(sym->sym.st_info);
> +	sym->bind = GELF_ST_BIND(sym->sym.st_info);
> +
> +	if ((sym->sym.st_shndx > SHN_UNDEF &&
> +	     sym->sym.st_shndx < SHN_LORESERVE) ||
> +	    (shndx != SHN_XINDEX && sym->sym.st_shndx == SHN_XINDEX)) {
> +		if (sym->sym.st_shndx != SHN_XINDEX)
> +			shndx = sym->sym.st_shndx;
> +
> +		sym->sec = find_section_by_index(elf, shndx);
> +		if (!sym->sec) {
> +			WARN("couldn't find section for symbol %s",
> +			     sym->name);
> +			return false;
> +		}

...

> @@ -366,47 +414,11 @@ static int read_symbols(struct elf *elf)
>  			goto err;
>  		}
>  
> -		sym->type = GELF_ST_TYPE(sym->sym.st_info);
> -		sym->bind = GELF_ST_BIND(sym->sym.st_info);
> +		if (!shndx_data)
> +			shndx = SHN_XINDEX;
>  
> -		if ((sym->sym.st_shndx > SHN_UNDEF &&
> -		     sym->sym.st_shndx < SHN_LORESERVE) ||
> -		    (shndx_data && sym->sym.st_shndx == SHN_XINDEX)) {
> -			if (sym->sym.st_shndx != SHN_XINDEX)
> -				shndx = sym->sym.st_shndx;
> -
> -			sym->sec = find_section_by_index(elf, shndx);
> -			if (!sym->sec) {
> -				WARN("couldn't find section for symbol %s",
> -				     sym->name);
> -				goto err;

...

> +	if (!elf_symbol_add(elf, sym, SHN_XINDEX)) {
> +		WARN("elf_symbol_add");
> +		return NULL;
> +	}

SHN_XINDEX means that the extended section index is used. Above you seem 
to use it in the opposite sense too (assigning to shndx when shndx_data is 
NULL). While it makes the code easier to handle, it is a bit confusing 
(and maybe I am just confused now). Could you add a comment about that, 
please? elf_symbol_add() seems like a good place.

Miroslav