lkml.org 
[lkml]   [2020]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC][PATCH 3/5] objtool: Add support for relocations without addends
From
Date


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@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?

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

\
 
 \ /
  Last update: 2020-05-13 18:58    [W:0.113 / U:0.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site