On Tue, Oct 18, 2022 at 4:19 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
Logging both is probably the easiest? Let's have them just in case,
shouldn't hurt; I'm not sure how relevant that really is..
> >
> > > 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.
SG, thanks!
> >
> > > 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
>
> [...]
