Re: [PATCH bpf-next v3 3/6] resolve_btfids: Introduce enum btf_id_kind

Ihor Solodrai
2025-12-16T02:31:29Z

On 12/11/25 11:09 PM, Eduard Zingerman wrote:
> On Fri, 2025-12-05 at 14:30 -0800, Ihor Solodrai wrote:
>> Instead of using multiple flags, make struct btf_id tagged with an
>> enum value indicating its kind in the context of resolve_btfids.
>>
>> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>> ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> (But see a question below).
>
>> @@ -213,14 +218,19 @@ btf_id__add(struct rb_root *root, char *name, bool unique)
>> p = &(*p)->rb_left;
>> else if (cmp > 0)
>> p = &(*p)->rb_right;
>> - else
>> - return unique ? NULL : id;
>> + else if (kind == BTF_ID_KIND_SYM && id->kind == BTF_ID_KIND_SYM)
>
> Nit: I'd keep the 'unique' parameter alongside 'kind' and resolve this
> condition on the function callsite.

I don't like the boolean args, they're always opaque on the callsite.

We want to allow duplicates for _KIND_SYM and forbid for other kinds.
Since we are passing the kind from outside, I think it makes sense to
check for this inside the function. It makes the usage simpler.

>
>> + return id;
>> + else {
>> + pr_err("Unexpected duplicate symbol %s of kind %d\n", name, id->kind);
>> + return NULL;
>> + }
>
> [...]
>
>> @@ -491,28 +515,24 @@ static int symbols_collect(struct object *obj)
>> id = add_symbol(&obj->funcs, prefix, sizeof(BTF_FUNC) - 1);
>> /* set8 */
>> } else if (!strncmp(prefix, BTF_SET8, sizeof(BTF_SET8) - 1)) {
>> - id = add_set(obj, prefix, true);
>> + id = add_set(obj, prefix, BTF_ID_KIND_SET8);
>> /*
>> * SET8 objects store list's count, which is encoded
>> * in symbol's size, together with 'cnt' field hence
>> * that - 1.
>> */
>> - if (id) {
>> + if (id)
>> id->cnt = sym.st_size / sizeof(uint64_t) - 1;
>> - id->is_set8 = true;
>> - }
>> /* set */
>> } else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
>> - id = add_set(obj, prefix, false);
>> + id = add_set(obj, prefix, BTF_ID_KIND_SET);
>> /*
>> * SET objects store list's count, which is encoded
>> * in symbol's size, together with 'cnt' field hence
>> * that - 1.
>> */
>> - if (id) {
>> + if (id)
>
> Current patch is not a culprit, but shouldn't resolve_btfids fail if
> `id` cannot be added? (here and in a hunk above).

By the existing design, resolve_btfids generally fails if
CONFIG_WERROR is set and `warnings > 0`.

And in this particular place it would fails with -ENOMEM a bit below:

[...]
} else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
id = add_set(obj, prefix, BTF_ID_KIND_SET);
/*
* SET objects store list's count, which is encoded
* in symbol's size, together with 'cnt' field hence
* that - 1.
*/
if (id)
id->cnt = sym.st_size / sizeof(int) - 1;
} else {
pr_err("FAILED unsupported prefix %s\n", prefix);
return -1;
}

/* --> */ if (!id)
return -ENOMEM;

So I think an error code change may be appropriate, and that's about it.

>
>> id->cnt = sym.st_size / sizeof(int) - 1;
>> - id->is_set = true;
>> - }
>> } else {
>> pr_err("FAILED unsupported prefix %s\n", prefix);
>> return -1;
>
> [...]
>