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;
Subject: Re: [RFC][PATCH 3/5] objtool: Add support for relocations without
 addends
To:     Matt Helsley <mhelsley@vmware.com>, linux-kernel@vger.kernel.org
Cc:     Josh Poimboeuf <jpoimboe@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Miroslav Benes <mbenes@suse.cz>,
        Steven Rostedt <rostedt@goodmis.org>
References: <cover.1588888003.git.mhelsley@vmware.com>
 <17ee3f6f2a246008aaae70f92df24ae92fa0e21e.1588888003.git.mhelsley@vmware.com>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <f79a3872-b9ad-a966-71fb-4451e0901281@redhat.com>
Date:   Tue, 12 May 2020 18:04:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <17ee3f6f2a246008aaae70f92df24ae92fa0e21e.1588888003.git.mhelsley@vmware.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Matt,

On 5/11/20 6:35 PM, 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)
>   	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;
>   
> -		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));
>   		if (!sec->base) {
> -			WARN("can't find base section for rela section %s",
> +			WARN("can't find base section for relocation section %s",
>   			     sec->name);
>   			return -1;
>   		}
> @@ -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;
>   			}
>   
>   			rela->type = GELF_R_TYPE(rela->rela.r_info);
> -			rela->addend = rela->rela.r_addend;
>   			rela->offset = rela->rela.r_offset;
>   			symndx = GELF_R_SYM(rela->rela.r_info);
>   			rela->sym = find_symbol_by_index(elf, symndx);
> @@ -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);
> +	}

This looks a bit error prone to me.

What about having:

     void *rel_buf;
     [...]
     size = nr * (sec->sh.sh_type == SHT_REL ? sizeof(GElf_Rel) : 
sizeof(GElf_Rela));
     rel_buf = malloc(size);
     [...]

And then casting rel_buf to the correct pointer type in the fitting 
switch cases?

>   
>   	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++;
>   	}
>   
> diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
> index 0b79c2353a21..71bd47055963 100644
> --- a/tools/objtool/elf.h
> +++ b/tools/objtool/elf.h
> @@ -61,7 +61,10 @@ struct symbol {
>   struct rela {
>   	struct list_head list;
>   	struct hlist_node hash;
> -	GElf_Rela rela;
> +	union {
> +		GElf_Rela rela;
> +		GElf_Rel  rel;
> +	};
>   	struct section *sec;
>   	struct symbol *sym;
>   	unsigned int type;
> 

-- 
Julien Thierry