Re: [PATCH v4 03/10] objtool: Handle different entry size of rodata

[Tiezhu Yang]

On 11/26/2024 06:59 PM, Tiezhu Yang wrote:
> On 11/26/2024 03:02 PM, Josh Poimboeuf wrote:
> > On Fri, Nov 22, 2024 at 12:49:58PM +0800, Tiezhu Yang wrote:
> > > @@ -2107,8 +2112,15 @@ static int add_jump_table(struct objtool_file
> > > *file, struct instruction *insn,
> > >          if (reloc != table && reloc == next_table)
> > >              break;
> > >
> > > +        /* Handle the special cases compiled with Clang on LoongArch */
> >
> > This comment is not helpful at all.  A comment is only needed if the
> > code is not already obvious.  In that case it should describe what is
> > being done and why.
>
> Will remove it.
>
> > > +        if (file->elf->ehdr.e_machine == EM_LOONGARCH &&
> > > +            reloc_type(reloc) == R_LARCH_32_PCREL)
> > > +            rodata_entry_size = 4;
> > > +        else
> > > +            rodata_entry_size = 8;
> >
> > Is this really loongarch-specific or is it only related to the size of
> > the reloc?
>
> This is related with the reloc type, but it may be wrong to only
> check the reloc type to assign the value of rodata_entry_size,
> becasue the value of reloc type for different archs may be same,
> so it needs to check ehdr.e_machine and relocation type.
>
> > Can this be abstracted out to a reloc_size() function like
> > so?
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/tree/tools/objtool/klp-diff.c?h=objtool-diff#n834
> >
> >
> > maybe it could live in elf.h.
>
> OK, if I understand your comment correctly, this should be an
> arch-specific function defined in
> tools/objtool/arch/*/include/arch/elf.h, otherwise it also needs to
> check ehdr.e_machine
> in tools/objtool/include/objtool/elf.h.

There are only macro definitions in
tools/objtool/arch/*/include/arch/elf.h,
so I think it is better to add reloc_size() in
tools/objtool/include/objtool/elf.h,
like this:

static inline unsigned int reloc_size(struct elf *elf, struct reloc *reloc)
{
        if (elf->ehdr.e_machine == EM_X86_64) {
                switch (reloc_type(reloc)) {
                case R_X86_64_32:
                case R_X86_64_PC32:
                        return 4;
                case R_X86_64_64:
                case R_X86_64_PC64:
                        return 8;
                default:
                        ERROR("unknown X86_64 reloc type");
                }
        } else if (elf->ehdr.e_machine == EM_LOONGARCH) {
                switch (reloc_type(reloc)) {
                case R_LARCH_32:
                case R_LARCH_32_PCREL:
                        return 4;
                case R_LARCH_64:
                case R_LARCH_64_PCREL:
                        return 8;
                default:
                        ERROR("unknown LoongArch reloc type");
                }
        } else {
                return 8;
        }
}

and call it in check.c, like this:

rodata_entry_size = reloc_size(file->elf, reloc);

What do you think?

Thanks,
Tiezhu