Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols

From: Yonghong Song <yhs@fb.com>
To: Hengqi Chen <hengqi.chen@gmail.com>, <bpf@vger.kernel.org>
Cc: <ast@kernel.org>, <daniel@iogearbox.net>, <andrii@kernel.org>,
	<john.fastabend@gmail.com>, <jolsa@kernel.org>,
	<yanivagman@gmail.com>
Subject: Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols
Date: Sun, 25 Jul 2021 20:32:44 -0700	[thread overview]
Message-ID: <dde36573-f6b9-8570-0878-e313e771345a@fb.com> (raw)
In-Reply-To: <20210725141814.2000828-2-hengqi.chen@gmail.com>

On 7/25/21 7:18 AM, Hengqi Chen wrote:
> Kernel functions referenced by .BTF_ids may changed from global to static
> and get inlined and thus disappears from BTF. This causes kernel build

the function could be renamed or removed too.

> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux.
> Update resolve_btfids to emit warning messages and patch zero id for missing
> symbols instead of aborting kernel build process.
> 
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>

LGTM with one minor comment below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/bpf/resolve_btfids/main.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 3ad9301b0f00..3ea19e33250d 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh)
>   	sh->sh_addralign = expected;
> 
>   	if (gelf_update_shdr(scn, sh) == 0) {
> -		printf("FAILED cannot update section header: %s\n",
> +		pr_err("FAILED cannot update section header: %s\n",
>   			elf_errmsg(-1));
>   		return -1;
>   	}
> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj)
> 
>   	elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL);
>   	if (!elf) {
> +		close(fd);
>   		pr_err("FAILED cannot create ELF descriptor: %s\n",
>   			elf_errmsg(-1));
>   		return -1;
> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj)
>   	err = libbpf_get_error(btf);
>   	if (err) {
>   		pr_err("FAILED: load BTF from %s: %s\n",
> -			obj->path, strerror(-err));
> +			obj->btf ?: obj->path, strerror(-err));

Why you change "obj->path" to "obj->btf ?: obj->path"?
Note that obj->path cannot be NULL.

>   		return -1;
>   	}
> 
[...]