On Wed, Nov 27, 2024 at 12:03:59PM +0000, Vadim Fedorenko wrote:
> On 27/11/2024 11:00, Jiri Olsa wrote:
> > On Tue, Nov 26, 2024 at 05:50:06PM -0800, Eduard Zingerman wrote:
> > > btf_encoder__tag_kfuncs() reads .BTF_ids section to identify a set of
> > > kfuncs present in the ELF file being processed.
> > > This section consists of:
> > > - arrays of uint32_t elements;
> > > - arrays of records with the following structure:
> > > struct btf_id_and_flag {
> > > uint32_t id;
> > > uint32_t flags;
> > > };
> > >
> > > When endianness of a binary operated by pahole differs from the host
> > > system's endianness, these fields require byte-swapping before use.
> > > Currently, this byte-swapping does not occur, resulting in kfuncs not
> > > being marked with declaration tags.
> > >
> > > This commit resolves the issue by using elf_getdata_rawchunk()
> > > function to read .BTF_ids section data. When called with ELF_T_WORD as
> > > 'type' parameter it does necessary byte order conversion
> > > (only if host and elf endianness do not match).
> > >
> > > Cc: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > > Cc: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > Cc: Daniel Xu <dxu@xxxxxxxxx>
> > > Cc: Jiri Olsa <olsajiri@xxxxxxxxx>
> > > Cc: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > Cc: Vadim Fedorenko <vadfed@xxxxxxxx>
> > > Fixes: 72e88f29c6f7 ("pahole: Inject kfunc decl tags into BTF")
> > > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > > ---
> > > btf_encoder.c | 26 ++++++++++++++++++++------
> > > 1 file changed, 20 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > index e1adddf..3754884 100644
> > > --- a/btf_encoder.c
> > > +++ b/btf_encoder.c
> > > @@ -1904,18 +1904,32 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
> > > goto out;
> > > }
> > > - data = elf_getdata(scn, 0);
> > > - if (!data) {
> > > - elf_error("Failed to get ELF section(%d) data", i);
> > > - goto out;
> > > - }
> > > -
> > > if (shdr.sh_type == SHT_SYMTAB) {
> > > + data = elf_getdata(scn, 0);
> > > + if (!data) {
> > > + elf_error("Failed to get ELF section(%d) data", i);
> > > + goto out;
> > > + }
> > > +
> > > symbols_shndx = i;
> > > symscn = scn;
> > > symbols = data;
> > > strtabidx = shdr.sh_link;
> > > } else if (!strcmp(secname, BTF_IDS_SECTION)) {
> > > + /* .BTF_ids section consists of uint32_t elements,
> > > + * and thus might need byte order conversion.
> > > + * However, it has type PROGBITS, hence elf_getdata()
> > > + * won't automatically do the conversion.
> > > + * Use elf_getdata_rawchunk() instead,
> > > + * ELF_T_WORD tells it to do the necessary conversion.
> > > + */
> > > + data = elf_getdata_rawchunk(elf, shdr.sh_offset, shdr.sh_size, ELF_T_WORD);
> >
> > looks good, I'm just curious about one thing..
> >
> > so ELF_T_WORD enum has this comment: /* Elf32_Word, Elf64_Word, ... */
> >
> > I did just quick check, ***so I might be easily wrong***, but I wonder the
> > code in __elf_xfctstom (which I assume is the one called for conversion)
> > chooses to swap 32/64 bits values based on elf->class .. so for 64bit ELF
> > class we swap 64bit values? ... while .BTF_ids has always 32 bit values
>
> Well according to the doc:
>
> ELF_T_WORD Unsigned 32-bit words.
> ELF_T_XWORD Unsigned 64-bit words.
>
> It shouldn't use 64 bits swap:
>
> const xfct_t __elf_xfctstom[EV_NUM - 1][EV_NUM - 1][ELFCLASSNUM -
> 1][ELF_T_NUM] =
> ....
> [ELF_T_WORD] = ElfW2(Bits, cvt_Word),
> [ELF_T_XWORD] = ElfW2(Bits, cvt_Xword),
> ...
>
> Are you looking somewhere else?
nah I guess I got confused with Elf64_Word, which is still 32bits,
seems fine, sorry for noise
Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
thanks,
jirka
