Re: duplicate BTF_IDs leading to symbol redefinition errors?

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Sep 15, 2023 at 01:41:25PM -0700, Andrii Nakryiko wrote:
> On Fri, Sep 15, 2023 at 1:28 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Sep 14, 2023 at 11:14:19AM -0700, Andrii Nakryiko wrote:
> > > On Thu, Sep 14, 2023 at 2:52 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Thu, Sep 14, 2023 at 05:30:51PM +0900, Masahiro Yamada wrote:
> > > >
> > > > SNIP
> > > >
> > > > > > > so the change is about adding unique id that's basically path of
> > > > > > > the object stored in base32 so it could be used as symbol, so we
> > > > > > > don't really need to read the actual file
> > > > > > >
> > > > > > > the problem is when BTF_ID definition like:
> > > > > > >
> > > > > > > BTF_ID(struct, cgroup)
> > > > > > >
> > > > > > > translates in 2 separate objects into same symbol name because of
> > > > > > > the matching __COUNTER__ macro values (like 380 below)
> > > > > > >
> > > > > > >   __BTF_ID__struct__cgroup__380
> > > > > > >
> > > > > > > this change just adds unique id of the path name at the end of the
> > > > > > > symbol with:
> > > > > > >
> > > > > > >   echo -n 'kernel/bpf/helpers' | base32 -w0 --> NNSXE3TFNQXWE4DGF5UGK3DQMVZHG
> > > > > > >
> > > > > > > so the symbol looks like:
> > > > > > >
> > > > > > >   __BTF_ID__struct__cgroup__380NNSXE3TFNQXWE4DGF5UGK3DQMVZHG
> > > > > > >
> > > > > > > and is unique over the sources
> > > > > > >
> > > > > > > but I still hope we could come up with some better solution ;-)
> > > > > >
> > > > > > so far the only better solution I could come up with is to use
> > > > > > cksum (also from coreutils) instead of base32, which makes the
> > > > > > BTF_ID_BASE value compact
> > > > > >
> > > > > > I'll run test to find out how much it hurts the build time
> > > > > >
> > > > > > jirka
> > > > >
> > > > >
> > > > >
> > > > > Seems a bad idea to me.
> > > > >
> > > > > It would fork a new shell and chsum for all files,
> > > > > while only a few of them need it.
> > > >
> > > > right, I have a change to limit this on kernel and net directories,
> > > > but it's still bad
> > > >
> > > > >
> > > > > Better to consult BTF forks.
> > > >
> > > > perhaps there's better way within kbuild to get unique id/value
> > > > for each object file?
> > >
> > > let's just use __LINE__ + __COUNTER__ for now and teach resolve_btfids
> > > to fail and complain loudly about duplicate symbols?
> >
> > ok, will send that.. but it fails during link before resolve_btfids
> > takes place
> >
> > >
> > >
> > > This will give us time and opportunity to implement a better approach
> > > to .BTF_ids overall. Encoding the desired type name in the symbol name
> > > always felt off. Maybe it's better to encode type + name as data,
> > > which is discarded at the latest stage during vmlinux linking? Either
> >
> > hum, so maybe having a special section (.BTF_ids_desc below)
> > that would have record for each ID placed in .BTF_ids section:
> >
> > asm(                                           \
> > ".pushsection .BTF_ids,\"a\";        \n"       \
> > "1:                                  \n"       \
> > ".zero 4                             \n"       \
> > ".popsection;                        \n"       \
> > ".pushsection .BTF_ids_desc,\"a\";   \n"       \
> > ".long 1b                            \n"       \
> >
> > and somehow get prefix and name pointers in here:
> >
> > ".long prefix
> > ".long name
> >
> > ".popsection;                        \n");
> >
> > so resolve_btfids would iterate .BTF_ids_desc records and fix
> > up .BTF_ids data..
> >
> 
> Something like that. I don't think it's even a regression in terms of
> vmlinux space usage, because right now we spend as much space on
> storing symbol names. So just adding string pointers would be already
> a win due to repeating "struct", "func", etc strings.
> 
> 
> > we might need to do one extra link phase to get rid of the
> > .BTF_ids_desc secion
> 
> Hopefully we can find a way to avoid this, we already do like 3 link
> phases at least (for kallsyms), so doing all that on the first one and
> then stripping it out using link script at subsequent one would be
> best.

perhaps we could move that section under .init.data and
get rid of it on startup

jirka