Re: [PATCH bpf-next v2 04/13] resolve_btfids: Introduce finalize_btf() step

[Ihor Solodrai]

On 1/20/26 10:19 AM, Eduard Zingerman wrote:
> On Tue, 2026-01-20 at 10:11 -0800, Ihor Solodrai wrote:
> 
> [...]
> 
>>>> @@ -1099,12 +1116,22 @@ int main(int argc, const char **argv)
>>>>  	if (obj.efile.idlist_shndx == -1 ||
>>>>  	    obj.efile.symbols_shndx == -1) {
>>>>  		pr_debug("Cannot find .BTF_ids or symbols sections, skip symbols resolution\n");
>>>> -		goto dump_btf;
>>>> +		resolve_btfids = false;
>>>>  	}
>>>>  
>>>> -	if (symbols_collect(&obj))
>>>> +	if (resolve_btfids)
>>>> +		if (symbols_collect(&obj))
>>>> +			goto out;
>>>
>>> Nit: check obj.efile.idlist_shndx and obj.efile.symbols_shndx inside symbols_collect()?
>>>      To avoid resolve_btfids flag and the `goto dump_btf;` below.
>>
>> Hi Eduard, thank you for review.
>>
>> The issue is that in case of .BTF_ids section absent we have to skip
>> some of the steps, specifically:
>>   - symbols_collect()
>>   - sequence between symbols_resolve() and dump_raw_btf_ids()
> 
>> It's not an exit condition, we still have to do load/dump of the BTF.
>>
>> I tried in symbols_collect():
>>
>> 	if (obj.efile.idlist_shndx == -1 || obj.efile.symbols_shndx == -1)
>> 		return 0;
>>
>> But then, we either have to do the same check in symbols_resolve() and
>> co, or maybe store a flag in the struct object.  So I decided it's
>> better to have an explicit flag in the main control flow, instead of
>> hiding it.
> 
> For symbols_resolve() is any special logic necessary?
> I think that `id = btf_id__find(root, str);` will just return NULL for
> every type, thus the whole function would be a noop passing through
> BTF types once.
> 
> symbols_patch() will be a noop, as it will attempt traversing empty roots.
> dump_raw_btf_ids() already returns if there are no .BTF_ids.

Hm... Looks like you're right, those would be noops.

Still, I think it's clearer what steps are skipped with a toplevel
flag.  Otherwise to figure out that those are noops you need to check
every subroutine (as you just did), and a future change may
unintentionally break the expectation of noop creating an unnecessary
debugging session.

And re symbols_resolve(), if we don't like allocating unnecessary
memory, why are we ok with traversing the BTF with noops? Seems
a bit inconsistent to me.

> 
> [...]