On Tue, Oct 18, 2022 at 11:47 AM <sdf@xxxxxxxxxx> wrote:
>
> On 10/17, Andrii Nakryiko wrote:
> > Refactor libbpf's BTF fixup step during BPF object open phase. The only
> > functional change is that we now ignore BTF_VAR_GLOBAL_EXTERN variables
> > during fix up, not just BTF_VAR_STATIC ones, which shouldn't cause any
> > change in behavior as there shouldn't be any extern variable in data
> > sections for valid BPF object anyways.
>
> > Otherwise it's just collapsing two functions that have no reason to be
> > separate, and switching find_elf_var_offset() helper to return entire
> > symbol pointer, not just its offset. This will be used by next patch to
> > get ELF symbol visibility.
>
> > While refactoring, also "normalize" debug messages inside
> > btf_fixup_datasec() to follow general libbpf style and print out data
> > section name consistently, where it's available.
>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>
> Left a couple of questions below.
>
> > ---
> > tools/lib/bpf/libbpf.c | 95 ++++++++++++++++++------------------------
> > 1 file changed, 41 insertions(+), 54 deletions(-)
>
[...]
> > - /* .extern datasec size and var offsets were set correctly during
> > - * extern collection step, so just skip straight to sorting variables
> > + /* extern-backing datasecs (.ksyms, .kconfig) have their size and
> > + * variable offsets set at the previous step, so we skip any fixups
> > + * for such sections
> > */
> > if (t->size)
> > goto sort_vars;
>
> > - ret = find_elf_sec_sz(obj, name, &size);
> > - if (ret || !size) {
> > - pr_debug("Invalid size for section %s: %u bytes\n", name, size);
> > + err = find_elf_sec_sz(obj, sec_name, &size);
> > + if (err || !size) {
> > + pr_debug("sec '%s': invalid size %u bytes\n", sec_name, size);
>
> nit: do we want to log err instead here? it seems like the size will be
> zero on error anyway, so probably not worth logging it?
hmm.. I mostly just preserved the original message content. Error can
be zero, and size can be zero, so don't know, we can log both or none?
Section name will probably be more important in practice.
>
> > return -ENOENT;
> > }
>
> > t->size = size;
>
> > for (i = 0, vsi = btf_var_secinfos(t); i < vars; i++, vsi++) {
> > + const struct btf_type *t_var;
> > + struct btf_var *var;
> > + const char *var_name;
> > + Elf64_Sym *sym;
> > +
[...]
> > -static int btf_finalize_data(struct bpf_object *obj, struct btf *btf)
> > +static int bpf_object_fixup_btf(struct bpf_object *obj)
> > {
> > - int err = 0;
> > - __u32 i, n = btf__type_cnt(btf);
> > + int i, n, err = 0;
>
> > + if (!obj->btf)
> > + return 0;
> > +
> > + n = btf__type_cnt(obj->btf);
>
> qq: why do s/__u32/int/ here? btf__type_cnt seems to be returning u32?
mostly to consolidate all the variables above into single short line.
BTF IDs can't be so big to not fit into int, so it's safe to use
signed int everywhere.
>
> > for (i = 1; i < n; i++) {
> > - struct btf_type *t = btf_type_by_id(btf, i);
> > + struct btf_type *t = btf_type_by_id(obj->btf, i);
>
> > /* Loader needs to fix up some of the things compiler
> > * couldn't get its hands on while emitting BTF. This
[...]
