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 <[email protected]>
> > ---
> > 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
