Re: [PATCH bpf-next 05/10] libbpf: add BPF static linker APIs

On Wed, Mar 10, 2021 at 6:34 PM Alexei Starovoitov
[off-list ref] wrote:

On Tue, Mar 09, 2021 at 08:04:26PM -0800, Andrii Nakryiko wrote:

quoted

+
+     struct btf *strtab_btf; /* we use struct btf to manage strings */

...

quoted

+     str_off = btf__add_str(linker->strtab_btf, sec->sec_name);
+     sec->shdr->sh_name = str_off;

That bit took me an hour to grok.
That single line comment above is far far from obvious.

Heh, I guess I've been working with BTF, ELF and pahole for too long
to notice that it's so non-obvious. pahole wraps `struct btf` in a
similar fashion for deduplicated string management.

What the logic is relying on is that string section in BTF format
has the same zero terminated set of strings as ELF's .strtab section.
There is no BTF anywhere here in this 'strtab_btf'.
The naming choice made it double hard.

Right. strtab_strs would probably be a slightly better choice.

My understanding that you're using that instead of renaming btf_add_mem()
into something generic to rely on string hashmap for string dedup?

It's not about renaming btf_add_mem(). btf_add_mem() just implements
memory re-allocation (with exponential increase). But here we want to
not add a new string if it's already present. So it's much more
complicated logic than btf_add_mem().

The commit log in patch 2 that introduces btf_raw_strs() sort of talks about
this code puzzle, but I would never guessed that's what you meant based
on patch 2 alone.

Did you consider some renaming/generalizing of string management to
avoid btf__add_str() through out the patch 5?
The "btf_" prefix makes things challenging to read.
Especially when patch 6 is using btf__add_str() to add to real BTF.

Right. I guess we can extract the "set of strings" data structure out
of `struct btf` into libbpf-internal data structure. Then use it from
struct btf and separately (and directly) from struct bpf_linker. I'll
see what that would involve in terms of refactoring.

Mainly pointing it out for others who might be looking at the patches.

That's a good point, I should have probably at least mentioned that
bit more explicitly.