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

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Yonghong Song <yhs@fb.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.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: Fri, 23 Apr 2021 16:35:09 -0700	[thread overview]
Message-ID: <CAADnVQLBaG9Q+xEcDqLDa2prn-7Fs2gqAiYsnQTz9CRBsE6uHw@mail.gmail.com> (raw)
In-Reply-To: <588c763f-1383-d92b-116a-c6826ffa1418@fb.com>

On Fri, Apr 23, 2021 at 4:26 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/23/21 4:05 PM, Alexei Starovoitov wrote:
> > On Fri, Apr 23, 2021 at 2:56 PM Yonghong Song <yhs@fb.com> wrote:
> >>>>>
> >>>>> -static volatile const __u32 print_len;
> >>>>> -static volatile const __u32 ret1;
> >>>>> +volatile const __u32 print_len = 0;
> >>>>> +volatile const __u32 ret1 = 0;
> >>>>
> >>>> I am little bit puzzled why bpf_iter_test_kern4.c is impacted. I think
> >>>> this is not in a static link test, right? The same for a few tests below.
> >>>
> >>> All the selftests are passed through a static linker, so it will
> >>> append obj_name to each static variable. So I just minimized use of
> >>> static variables to avoid too much code churn. If this variable was
> >>> static, it would have to be accessed as
> >>> skel->rodata->bpf_iter_test_kern4__print_len, for example.
> >>
> >> Okay this should be fine. selftests/bpf specific. I just feel that
> >> some people may get confused if they write/see a single program in
> >> selftest and they have to use obj_varname format and thinking this
> >> is a new standard, but actually it is due to static linking buried
> >> in Makefile. Maybe add a note in selftests/README.rst so we
> >> can point to people if there is confusion.
> >
> > I'm not sure I understand.
> > Are you saying that
> > bpftool gen object out_file.o in_file.o
> > is no longer equivalent to llvm-strip ?
>
> This is more about BTF and ELF.
> Give a simple example,
> $ cat t1.c
> volatile static int aa = 10;
> int foo() { return aa; }
> $ clang -O2 -g -c -target bpf t1.c
>
> Using bpftool compiled with this patch:
> $ bpftool gen object output.o t1.o
> $ llvm-readelf -s t1.o | grep aa
>       3: 0000000000000000     4 OBJECT  LOCAL  DEFAULT     4 aa
> $ llvm-readelf -s output.o | grep aa
>       3: 0000000000000000     4 OBJECT  LOCAL  DEFAULT     4 aa
>
> $ bpftool btf dump file t1.o | grep aa
> [5] VAR 'aa' type_id=4, linkage=static
> $ bpftool btf dump file output.o | grep aa
> [5] VAR 't1..aa' type_id=4, linkage=static

Right. That's how I understood it.

> So yes you are right, this will affect skeleton user
> if you use static linker with single file.
>
> > Since during that step static vars will get their names mangled?
> > So a good chunk of code that uses skeleton right now should either
> > 1. don't do the linking step
> > or
> > 2. adjust their code to use global vars
> > or
> > 3. adjust the usage of skel.h in their corresponding user code
> >    to accommodate mangled static names?
> > Did it get it right?

It feels that the strategy of mangling every static name is too harsh.
Maybe sub-skeleton is an answer?
Something that would do a sub-skeleton for each file?
Mainly to keep "bpftool gen object output.o t1.o" from messing with btf names.
Maybe linking of btf-s from multiple .o should somehow scope them?
I don't have a concrete suggestion yet.