Re: [PATCH v5 bpf-next 1/9] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

Jiri Olsa
2020-07-07T15:43:55Z

On Mon, Jul 06, 2020 at 05:34:52PM -0700, Andrii Nakryiko wrote:

SNIP

> > +$(OUTPUT)rbtree.o: ../../lib/rbtree.c FORCE
> > + $(call rule_mkdir)
> > + $(call if_changed_dep,cc_o_c)
> > +
> > +$(OUTPUT)zalloc.o: ../../lib/zalloc.c FORCE
> > + $(call rule_mkdir)
> > + $(call if_changed_dep,cc_o_c)
> > +
> > +$(OUTPUT)string.o: ../../lib/string.c FORCE
> > + $(call rule_mkdir)
> > + $(call if_changed_dep,cc_o_c)
> > +
> > +$(OUTPUT)ctype.o: ../../lib/ctype.c FORCE
> > + $(call rule_mkdir)
> > + $(call if_changed_dep,cc_o_c)
> > +
> > +$(OUTPUT)str_error_r.o: ../../lib/str_error_r.c FORCE
> > + $(call rule_mkdir)
> > + $(call if_changed_dep,cc_o_c)
>
> Is Build also a Makefile? If that's the case, why not:
>
> $(output)%.o: ../../lib/%.c FORCE
> $(call rule_mkdir)
> $(call if_changed_dep,cc_o_c)

hum, it is ... I'll try that

>
> ?
>
> > diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
> > new file mode 100644
> > index 000000000000..948378ca73d4
> > --- /dev/null
> > +++ b/tools/bpf/resolve_btfids/Makefile
> > @@ -0,0 +1,77 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +include ../../scripts/Makefile.include
> > +
> > +ifeq ($(srctree),)
> > +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> > +srctree := $(patsubst %/,%,$(dir $(srctree)))
> > +srctree := $(patsubst %/,%,$(dir $(srctree)))
> > +endif
> > +
> > +ifeq ($(V),1)
> > + Q =
> > + msg =
> > +else
> > + Q = @
> > + msg = @printf ' %-8s %s%s\n' "$(1)" "$(notdir $(2))" "$(if $(3), $(3))";
> > + MAKEFLAGS=--no-print-directory
> > +endif
> > +
> > +OUTPUT ?= $(srctree)/tools/bpf/resolve_btfids/
>
> Ok, so this builds nicely for in-tree build, but when I did
> out-of-tree build (I use KBUILD_OUTPUT, haven't checked specifying
> O=whatever), I get:
>
> LD vmlinux
> BTFIDS vmlinux
> /data/users/andriin/linux/scripts/link-vmlinux.sh: line 342:
> /data/users/andriin/linux/tools/bpf/resolve_btfids/resolve_btfids: No
> such file or directory
>
> I suspect because you are assuming OUTPUT to be in srctree? You
> probably need to adjust for out-of-tree mode.

ok, make clean did not clean resolve_btfids, so it was
still there when I tried the out of the tree build..

>
> > +
> > +LIBBPF_SRC := $(srctree)/tools/lib/bpf/
> > +SUBCMD_SRC := $(srctree)/tools/lib/subcmd/
> > +
> > +BPFOBJ := $(OUTPUT)/libbpf.a
> > +SUBCMDOBJ := $(OUTPUT)/libsubcmd.a
>
> [...]
>
> > +
> > +#define BTF_IDS_SECTION ".BTF.ids"
>
> You haven't updated a bunch of places (cover letter, this patch commit
> message, maybe somewhere else) after renaming from .BTF_ids, please
> keep them in sync. Also, while I'm not too strongly against this name,
> it does sound like this section is part of generic BTF format (as is
> .BTF and .BTF.ext), which it is not, because it's so kernel-specific.
> So I'm mildly against it and pro .BTF_ids.

.BTF_ids it is.. I'll change all the places

>
> > +#define BTF_ID "__BTF_ID__"
> > +
> > +#define BTF_STRUCT "struct"
> > +#define BTF_UNION "union"
> > +#define BTF_TYPEDEF "typedef"
> > +#define BTF_FUNC "func"
> > +#define BTF_SET "set"
> > +
>
> [...]
>
> > +}
> > +
> > +static struct btf *btf__parse_raw(const char *file)
>
> I thought you were going to add this to libbpf itself? Or you planned
> to do a follow up patch later?

yes, I did not want to complicate this patchset more,
and send this change after.. also then there'll be one
more reason to make it a library function ;-)

thanks,
jirka