Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during linking

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andrii@kernel.org>,
	bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during linking
Date: Mon, 26 Apr 2021 15:34:49 -0700	[thread overview]
Message-ID: <20210426223449.5njjmcjpu63chqbb@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <CAEf4BzZOmCgmbYDUGA-s5AF6XJFkT1xKinY3Jax3Zm2OLNmguA@mail.gmail.com>

On Mon, Apr 26, 2021 at 08:44:04AM -0700, Andrii Nakryiko wrote:
> 
> >
> > > Static maps are slightly different, because we use SEC() which marks
> > > them as used, so they should always be present.
> >
> > yes. The used attribute makes the compiler keep the data,
> > but it can still inline it and lose the reference in the .text.
> 
> At least if the map is actually used with helpers (e.g.,
> bpf_map_lookup_elem(&map, ...)) it would be invalid for compiler to do
> anything crazy with that map reference, because compiler has no
> visibility into what opaque helpers do with that memory. So I don't
> think it can alias multiple maps, for instance. So I think static maps
> should be fine.

Yeah. That makes sense.

> See above about passing a pointer to map into black box functions. I'd
> bet that the compiler can't merge together two different references at
> least because of that.
> 
> For static maps, btw, just like for static functions and vars, there
> is no symbol, it's an offset into .maps section. We use that offset to
> identify the map itself.

Ok. Sounds like there is a desire to expose both static and static volatile
into skeleton.
Sure, but let's make it such the linking step doesn't change the skeleton.
Imagine a project that using single .bpf.c file and skeleton.
It grows and wants to split itself into multiple .bpf.c.
If such split would change the skeleton generated var/map names
it would be annoying user experience.

I see few options to avoid that:
- keeping the btf names as-is during linking
The final .o can have multiple vars and maps with the same name.
The skeleton gen can see the name collision and disambiguate them.
Here I think it's important to give users a choice. Blindly appending
file name is not ideal.
How to express it cleanly in .bpf.c? I don't know. SEC() would be a bit
ugly. May be similar to core flavors? ___1 and ___2 ? Also not ideal.
- another option is to fail skeleton gen if names conflict.
This way the users wold be able to link just fine and traditonal C style
linker behavior will be preserved, but if the user wants a skeleton
then the static map names across .bpf.c files shouldn't conflict.
imo that's reasonable restriction.
- maybe adopt __hidden for vars and maps? Only not hidden (which is default now)
would be seen in skeleton?