Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Matt Helsley <mhelsley@vmware.com>]

On Tue, May 12, 2020 at 06:04:50PM +0100, Julien Thierry wrote:
> 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));

I like reducing to a single size expression but I'm not a fan of hard-coding
the GElf_Rel[a] types here -- I prefer sizeof(*relas) and sizeof(*rels)
since that makes it clear the sizes will match the types of the pointers
that will be used to access them. So I've changed it to:

	size = nr * ((sec->sh.sh_type == SHT_REL) ? sizeof(*rels) : sizeof(*relas));

>     rel_buf = malloc(size);
>     [...]
> 
> And then casting rel_buf to the correct pointer type in the fitting switch
> cases?

I'm thinking it's simpler with fewer variables. I don't think
moving the cast into the switch cases makes it any clearer. It's also
odd because we'll keep re-initializing relas or rels to rel_buf each loop
iteration. Finally, this approach has the advantage that, when reviewing
the patch, it's clear that the original code handling RELA relocation entries
isn't changing -- you can see it's just shifting into one of the cases
(below).

Do you still prefer introducing rel_buf?

> 
> >   	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++;
> >   	}

Cheers,
     -Matt Helsley