Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 18 May 2022 09:40:42 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Josh Poimboeuf <jpoimboe@kernel.org>
Cc:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        llvm@lists.linux.dev, linux-kernel@vger.kernel.org,
        kasan-dev@googlegroups.com
Subject: Re: objtool "no non-local symbols" error with tip of tree LLVM
Message-ID: <20220518074042.GA10117@worktop.programming.kicks-ass.net>
References: <YoK4U9RgQ9N+HhXJ@dev-arch.thelio-3990X>
 <20220516214005.GQ76023@worktop.programming.kicks-ass.net>
 <YoPAZ6JfsF0LrQNc@hirez.programming.kicks-ass.net>
 <YoPCTEYjoPqE4ZxB@hirez.programming.kicks-ass.net>
 <20220518012429.4zqzarvwsraxivux@treble>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220518012429.4zqzarvwsraxivux@treble>
Precedence: bulk

On Tue, May 17, 2022 at 06:24:29PM -0700, Josh Poimboeuf wrote:
> On Tue, May 17, 2022 at 05:42:04PM +0200, Peter Zijlstra wrote:
> > +	for (;;) {
> > +		symtab_data = elf_getdata(s, symtab_data);
> > +		if (t)
> > +			shndx_data = elf_getdata(t, shndx_data);
> >  
> > -	sym->idx = symtab->sh.sh_size / sizeof(sym->sym);
> > -	elf_dirty_reloc_sym(elf, sym);
> > +		if (!symtab_data) {
> > +			if (!idx) {
> > +				void *buf;
> 
> I'm confused by whatever this is doing, how is !symtab_data possible,
> i.e. why would symtab not have data?
> 
> >  elf_create_section_symbol(struct elf *elf, struct section *sec)
> >  {
> >  	struct section *symtab, *symtab_shndx;
> > -	Elf_Data *shndx_data = NULL;
> > -	struct symbol *sym;
> > -	Elf32_Word shndx;
> > +	Elf32_Word first_non_local, new;
> > +	struct symbol *sym, *old;
> > +	int size;
> > +
> > +	if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
> > +		size = sizeof(Elf32_Sym);
> > +	else
> > +		size = sizeof(Elf64_Sym);
> 
> This should probably be called 'entsize' and I think you can just get it
> from symtab->sh.sh_entsize.
> 
> > +	/*
> > +	 * Either way, we added a LOCAL symbol.
> > +	 */
> > +	symtab->sh.sh_info += 1;
> > +
> >  	elf_add_symbol(elf, sym);
> 
> Not sure if it matters here, but elf_add_symbol() doesn't set sym->alias
> and sym->pv_target, and both of those are unconditionally initialized in
> read_symbols().  Should elf_add_symbol() be changed to initialize them?


--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -374,6 +374,9 @@ static void elf_add_symbol(struct elf *e
 	struct list_head *entry;
 	struct rb_node *pnode;

+	INIT_LIST_HEAD(&sym->pv_target);
+	sym->alias = sym;
+
 	sym->type = GELF_ST_TYPE(sym->sym.st_info);
 	sym->bind = GELF_ST_BIND(sym->sym.st_info);

@@ -438,8 +441,6 @@ static int read_symbols(struct elf *elf)
 			return -1;
 		}
 		memset(sym, 0, sizeof(*sym));
-		INIT_LIST_HEAD(&sym->pv_target);
-		sym->alias = sym;

 		sym->idx = i;

@@ -604,7 +605,8 @@ static void elf_dirty_reloc_sym(struct e

 /*
  * The libelf API is terrible; gelf_update_sym*() takes a data block relative
- * index value. As such, iterate the data blocks and adjust index until it fits.
+ * index value, *NOT* the symbol index. As such, iterate the data blocks and
+ * adjust index until it fits.
  *
  * If no data block is found, allow adding a new data block provided the index
  * is only one past the end.
@@ -613,14 +615,10 @@ static int elf_update_symbol(struct elf
 			     struct section *symtab_shndx, struct symbol *sym)
 {
 	Elf_Data *symtab_data = NULL, *shndx_data = NULL;
+	Elf64_Xword entsize = symtab->sh.sh_entsize;
 	Elf32_Word shndx = sym->sec->idx;
 	Elf_Scn *s, *t = NULL;
-	int size, idx = sym->idx;
-
-	if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
-		size = sizeof(Elf32_Sym);
-	else
-		size = sizeof(Elf64_Sym);
+	int max_idx, idx = sym->idx;

 	s = elf_getscn(elf->elf, symtab->idx);
 	if (!s) {
@@ -637,11 +635,14 @@ static int elf_update_symbol(struct elf
 	}

 	for (;;) {
+		/* get next data descriptor for the relevant sections */
 		symtab_data = elf_getdata(s, symtab_data);
 		if (t)
 			shndx_data = elf_getdata(t, shndx_data);

+		/* end-of-list */
 		if (!symtab_data) {
+			/* if @idx == 0, it's the next contiguous entry, create it */
 			if (!idx) {
 				void *buf;

@@ -649,53 +650,60 @@ static int elf_update_symbol(struct elf
 				if (t)
 					shndx_data = elf_newdata(t);

-				buf = calloc(1, size);
+				buf = calloc(1, entsize);
 				if (!buf) {
 					WARN("malloc");
 					return -1;
 				}

 				symtab_data->d_buf = buf;
-				symtab_data->d_size = size;
+				symtab_data->d_size = entsize;
 				symtab_data->d_align = 1;
 				symtab_data->d_type = ELF_T_SYM;

-				symtab->sh.sh_size += size;
+				symtab->sh.sh_size += entsize;
 				symtab->changed = true;

 				if (t) {
 					shndx_data->d_buf = &sym->sec->idx;
 					shndx_data->d_size = sizeof(Elf32_Word);
-					shndx_data->d_align = 4;
+					shndx_data->d_align = sizeof(Elf32_Word);
 					shndx_data->d_type = ELF_T_WORD;

-					symtab_shndx->sh.sh_size += 4;
+					symtab_shndx->sh.sh_size += sizeof(Elf32_Word);
 					symtab_shndx->changed = true;
 				}

 				break;
 			}

+			/* we don't do holes in symbol tables */
 			WARN("index out of range");
 			return -1;
 		}

+		/* empty blocks should not happen */
 		if (!symtab_data->d_size) {
 			WARN("zero size data");
 			return -1;
 		}

-		if (idx * size < symtab_data->d_size)
+		/* is this the right block? */
+		max_idx = symtab_data->d_size / entsize;
+		if (idx < max_idx)
 			break;

-		idx -= symtab_data->d_size / size;
+		/* adjust index and try again */
+		idx -= max_idx;
 	}

+	/* something went side-ways */
 	if (idx < 0) {
 		WARN("negative index");
 		return -1;
 	}

+	/* setup extended section index magic and write the symbol */
 	if (shndx >= SHN_UNDEF && shndx < SHN_LORESERVE) {
 		sym->sym.st_shndx = shndx;
 		if (!shndx_data)
@@ -720,14 +728,8 @@ static struct symbol *
 elf_create_section_symbol(struct elf *elf, struct section *sec)
 {
 	struct section *symtab, *symtab_shndx;
-	Elf32_Word first_non_local, new;
+	Elf32_Word first_non_local, new_idx;
 	struct symbol *sym, *old;
-	int size;
-
-	if (elf->ehdr.e_ident[EI_CLASS] == ELFCLASS32)
-		size = sizeof(Elf32_Sym);
-	else
-		size = sizeof(Elf64_Sym);

 	symtab = find_section_by_name(elf, ".symtab");
 	if (symtab) {
@@ -752,16 +754,15 @@ elf_create_section_symbol(struct elf *el
 	// st_value 0
 	// st_size 0

-	new = symtab->sh.sh_size / size;
-
 	/*
 	 * Move the first global symbol, as per sh_info, into a new, higher
 	 * symbol index. This fees up a spot for a new local symbol.
 	 */
 	first_non_local = symtab->sh.sh_info;
+	new_idx = symtab->sh.sh_size / symtab->sh.sh_entsize;
 	old = find_symbol_by_index(elf, first_non_local);
 	if (old) {
-		old->idx = new;
+		old->idx = new_idx;

 		hlist_del(&old->hash);
 		elf_hash_add(symbol, &old->hash, old->idx);
@@ -773,10 +774,10 @@ elf_create_section_symbol(struct elf *el
 			return NULL;
 		}

-		new = first_non_local;
+		new_idx = first_non_local;
 	}

-	sym->idx = new;
+	sym->idx = new_idx;
 	if (elf_update_symbol(elf, symtab, symtab_shndx, sym)) {
 		WARN("elf_update_symbol");
 		return NULL;