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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jul 3, 2020 at 2:52 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The resolve_btfids tool scans elf object for .BTF_ids section
> and resolves its symbols with BTF ID values.
>
> It will be used to during linking time to resolve arrays of BTF
> ID values used in verifier, so these IDs do not need to be
> resolved in runtime.
>
> The expected layout of .BTF_ids section is described in btfid.c
> header. Related kernel changes are coming in following changes.
>
> Build issue reported by 0-DAY CI Kernel Test Service.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/bpf/resolve_btfids/Build    |  26 ++
>  tools/bpf/resolve_btfids/Makefile |  77 ++++
>  tools/bpf/resolve_btfids/main.c   | 716 ++++++++++++++++++++++++++++++
>  3 files changed, 819 insertions(+)
>  create mode 100644 tools/bpf/resolve_btfids/Build
>  create mode 100644 tools/bpf/resolve_btfids/Makefile
>  create mode 100644 tools/bpf/resolve_btfids/main.c
>
> diff --git a/tools/bpf/resolve_btfids/Build b/tools/bpf/resolve_btfids/Build
> new file mode 100644
> index 000000000000..c7318cc55341
> --- /dev/null
> +++ b/tools/bpf/resolve_btfids/Build
> @@ -0,0 +1,26 @@
> +resolve_btfids-y += main.o
> +resolve_btfids-y += rbtree.o
> +resolve_btfids-y += zalloc.o
> +resolve_btfids-y += string.o
> +resolve_btfids-y += ctype.o
> +resolve_btfids-y += str_error_r.o
> +
> +$(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)

?

> 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.

> +
> +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.

> +#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?

> +{
> +       struct btf *btf;
> +       struct stat st;
> +       __u8 *buf;
> +       FILE *f;
> +
> +       if (stat(file, &st))
> +               return NULL;
> +
> +       f = fopen(file, "rb");
> +       if (!f)
> +               return NULL;
> +
> +       buf = malloc(st.st_size);
> +       if (!buf) {
> +               btf = ERR_PTR(-ENOMEM);
> +               goto exit_close;
> +       }
> +
> +       if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
> +               btf = ERR_PTR(-EINVAL);
> +               goto exit_free;
> +       }
> +
> +       btf = btf__new(buf, st.st_size);
> +
> +exit_free:
> +       free(buf);
> +exit_close:
> +       fclose(f);
> +       return btf;
> +}
> +

[...]