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

From: Yonghong Song <yhs@fb.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.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:26:27 -0700	[thread overview]
Message-ID: <588c763f-1383-d92b-116a-c6826ffa1418@fb.com> (raw)
In-Reply-To: <CAADnVQ+h9eS0P9Jb0QZQ374WxNSF=jhFAiBV7czqhnJxV51m6A@mail.gmail.com>

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

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?
> 