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

[Julien Thierry]

On 5/13/20 5:26 PM, Matt Helsley wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >    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?

On a completely personal taste, yes. I do not like having two local 
variables in the same scope pointing at the same data but with an 
implied "you should only use one or the other under the right 
circumstances".

But my main concern was having an allocation of a certain size and then 
modifying the size (might have been valid if sizeof(GElf_Rel) <= 
sizeof(GElf_Rela), but I must admit I did not bother to check). Since 
you've addressed that issue, the rest is just a matter of taste so 
better left to the maintainers.

Thanks,

--
Julien Thierry