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

[Jiri Olsa <jolsa@redhat.com>]

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