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 22:33:02 -0700	[thread overview]
Message-ID: <6863a87b-7121-79cb-76d9-6a02cd0acf33@fb.com> (raw)
In-Reply-To: <f1e7a60c-78ca-ce24-313b-c0e46507501a@gmail.com>

On 7/25/21 10:22 PM, Hengqi Chen wrote:
> 
> 
> On 2021/7/26 12:56 PM, Yonghong Song wrote:
>>
>>
>> On 7/25/21 9:41 PM, Hengqi Chen wrote:
>>>
>>>
>>> On 2021/7/26 11:32 AM, Yonghong Song wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> The diff didn't see the whole picture. Let me quote it here:
>>> ```
>>> btf = btf__parse(obj->btf ?: obj->path, NULL);
>>> err = libbpf_get_error(btf);
>>> if (err) {
>>>           pr_err("FAILED: load BTF from %s: %s\n",
>>>                   obj->path, strerror(-err));
>>>           return -1;
>>> }
>>> ```
>>>
>>> Because btf__parse parses either obj->btf or obj->path,
>>> I think the error message should reveal this.
>>
>> Okay, I see, obj->btf may not be NULL due to
>>                  OPT_STRING(0, "btf", &obj.btf, "BTF data",
>>                             "BTF data"),
>>
>> How about
>>    obj->btf ? "input BTF data" : obj->path
>>
>> The error message like
>>    FAILED: load BTF from : <error msg>
>> does not sound good.
>>
> 
> Sorry, I am confused.
> 
> If obj->btf is set, say, vmlinux.btf, the message should look like:
>   FAILED: load BTF from vmlinux.btf: <error msg>
> 
> Otherwise, it should look like:
>   FAILED: load BTF from vmlinux: <error msg>
> 
> Am I missing something ?

Ah, you are right. Your patch looks good. I didn't pay attention
and thought
   OPT_STRING(0, "btf", &obj.btf, "BTF data","BTF data")
is a string input for *btf data*, but actually it is actually
/sys/kernel/btf/vmlinux.

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