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 <[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)
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
