Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 May 2020 15:33:38 -0500
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Matt Helsley <mhelsley@vmware.com>
Cc:     linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Julien Thierry <jthierry@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC][PATCH 3/5] objtool: Add support for relocations without
 addends
Message-ID: <20200515203338.ehdgnvh7nqcczj4t@treble>
References: <cover.1588888003.git.mhelsley@vmware.com>
 <17ee3f6f2a246008aaae70f92df24ae92fa0e21e.1588888003.git.mhelsley@vmware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <17ee3f6f2a246008aaae70f92df24ae92fa0e21e.1588888003.git.mhelsley@vmware.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 11, 2020 at 10:35:11AM -0700, Matt Helsley wrote:
> Currently objtool only collects information about relocations with
> addends. In recordmcount, which we are about to merge into objtool,
> some supported architectures do not use rela relocations. Since
> object files use one or the other the list can be reused.
> 
> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> ---
>  tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
>  tools/objtool/elf.h |  5 ++++-
>  2 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index c4857fa3f1d1..cd841e3df87d 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)

This should probably be called read_relocs() now.

And 'struct rela' should probably be 'struct reloc'.  And I hate to say
it but all the 'rela' based variable/function names should also probably
be changed...

All the renaming might be disruptive for backports, but still I think it
would be a good idea.  It probably belongs in its own commit.  If it can
be done programmatically with 'sed -i' or so, with the exact command in
the commit log, even better :-)

>  	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
>  
>  	list_for_each_entry(sec, &elf->sections, list) {
> -		if (sec->sh.sh_type != SHT_RELA)
> +		if ((sec->sh.sh_type != SHT_RELA) &&
> +		     (sec->sh.sh_type != SHT_REL))
>  			continue;

The alignment is slightly off, should be:

		if ((sec->sh.sh_type != SHT_RELA) &&
		    (sec->sh.sh_type != SHT_REL))
			continue;

>  
> -		sec->base = find_section_by_name(elf, sec->name + 5);
> +		sec->base = find_section_by_name(elf, sec->name +
> +				((sec->sh.sh_type != SHT_REL) ? 5 : 4));

I think there's actually a cleaner way to do this, which we probably
should have been doing in the first place:

		sec->base = find_section_by_index(elf, sec->sh.sh_info);

(completely not tested, btw)

> @@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
>  			}
>  			memset(rela, 0, sizeof(*rela));
>  
> -			if (!gelf_getrela(sec->data, i, &rela->rela)) {
> -				WARN_ELF("gelf_getrela");
> -				return -1;
> +			switch(sec->sh.sh_type) {
> +			case SHT_REL:
> +				if (!gelf_getrel(sec->data, i, &rela->rel)) {
> +					WARN_ELF("gelf_getrel");
> +					return -1;
> +				}
> +				rela->addend = 0;
> +				break;
> +			case SHT_RELA:
> +				if (!gelf_getrela(sec->data, i, &rela->rela)) {
> +					WARN_ELF("gelf_getrela");
> +					return -1;
> +				}
> +				rela->addend = rela->rela.r_addend;
> +				break;
> +			default:
> +				break;

The default should never happen, but might as well return -1 for extra
robustness.

> @@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
>  	struct rela *rela;
>  	int nr, idx = 0, size;
>  	GElf_Rela *relas;
> +	GElf_Rel *rels;
>  
>  	nr = 0;
>  	list_for_each_entry(rela, &sec->rela_list, list)
>  		nr++;
>  
> +	/*
> +	 * Allocate a buffer for relocations with addends but also use
> +	 * it for other relocations too. The section type determines
> +	 * the size of the section, the buffer used, and the entries.
> +	 */
>  	size = nr * sizeof(*relas);
>  	relas = malloc(size);
>  	if (!relas) {
>  		perror("malloc");
>  		return -1;
>  	}
> +	rels = (void *)relas;
> +	if (sec->sh.sh_type == SHT_REL) {
> +		size = nr * sizeof(*rels);
> +	}
>  
>  	sec->data->d_buf = relas;
>  	sec->data->d_size = size;
> @@ -736,9 +761,19 @@ int elf_rebuild_rela_section(struct section *sec)
>  
>  	idx = 0;
>  	list_for_each_entry(rela, &sec->rela_list, list) {
> -		relas[idx].r_offset = rela->offset;
> -		relas[idx].r_addend = rela->addend;
> -		relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +		switch(sec->sh.sh_type) {
> +		case SHT_REL:
> +			rels[idx].r_offset = rela->offset;
> +			rels[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +			break;
> +		case SHT_RELA:
> +			relas[idx].r_addend = rela->addend;
> +			relas[idx].r_offset = rela->offset;
> +			relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +			break;
> +		default:
> +			break;
> +		}
>  		idx++;

There's a lot of trickiness going on here, in a valiant attempt to share
code, but really most of the code ends up not being shared anyway.

I think it would be a lot cleaner to just create a new "rel" version of
this function.

Then there could be a top-level

	elf_rebuild_reloc_section()

which calls the appropriate "rel" or "rela" variant.

-- 
Josh