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);
+ }
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;
--
2.20.1
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));
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
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 <[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?
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
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
On Wed, May 13, 2020 at 05:55:21PM +0100, Julien Thierry wrote:
>
>
> 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 <[email protected]>
> > > > ---
> > > > tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
> > > > tools/objtool/elf.h | 5 ++++-
<snip>
> > 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.
OK. As a sort of tie-breaker I tried to quickly find an analogous piece of code
in objtool which I could use as a guide on maintainer preference. The
only place I see void pointers being used is as keys for comparison
functions. So my guess and preference is to not use a void pointer here.
Cheers,
-Matt Helsley
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
On Fri, May 15, 2020 at 03:33:38PM -0500, Josh Poimboeuf wrote:
> 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;
>
Ack'd.
> >
> > - 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)
>
I can split that out as a separate patch so it's easy to test/drop.
> > @@ -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.
>
Ack'd. See below...
> > @@ -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.
OK, that all makes sense to me.
This can go after the multi-arch bits -- at the moment only recordmcount
will use this, though it might be useful for other archs if check or other
subcommands are made suitable for new archs before then.
So I'll move all of that work to a follow-on set rather than include it
in this multi-arch set. The next posting of this series will include all of
the other changes discussed and rebase on tip/master (to get Peter's changes).
The follow-on set after multi-arch support can:
1) Split the sec->base change into a separate patch
(since it's untested)
2) Rename patch (roughly starting with: sed -i 's/\brela\b/reloc/g')
3) Rebase this patch on top of that, making a new "reloc" version of
this function (rather than introduce something that needs
to be renamed)
Cheers,
-Matt Helsley