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>
---
tools/bpf/resolve_btfids/main.c | 76 ++++++++++++++++++++++-----------
1 file changed, 52 insertions(+), 24 deletions(-)
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index b4caae1170dd..cb1e69eb0bd7 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -98,6 +98,13 @@
# error "Unknown machine endianness!"
#endif
+enum btf_id_kind {
+ BTF_ID_KIND_NONE,
+ BTF_ID_KIND_SYM,
+ BTF_ID_KIND_SET,
+ BTF_ID_KIND_SET8
+};
+
struct btf_id {
struct rb_node rb_node;
char *name;
@@ -105,9 +112,8 @@ struct btf_id {
int id;
int cnt;
};
+ enum btf_id_kind kind;
int addr_cnt;
- bool is_set;
- bool is_set8;
Elf64_Addr addr[ADDR_CNT];
};
@@ -197,8 +203,7 @@ static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
return NULL;
}
-static struct btf_id *
-btf_id__add(struct rb_root *root, char *name, bool unique)
+static struct btf_id *btf_id__add(struct rb_root *root, char *name, enum btf_id_kind kind)
{
struct rb_node **p = &root->rb_node;
struct rb_node *parent = NULL;
@@ -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)
+ return id;
+ else {
+ pr_err("Unexpected duplicate symbol %s of kind %d\n", name, id->kind);
+ return NULL;
+ }
}
id = zalloc(sizeof(*id));
if (id) {
pr_debug("adding symbol %s\n", name);
id->name = name;
+ id->kind = kind;
rb_link_node(&id->rb_node, parent, p);
rb_insert_color(&id->rb_node, root);
}
@@ -260,22 +270,36 @@ static char *get_id(const char *prefix_end)
return id;
}
-static struct btf_id *add_set(struct object *obj, char *name, bool is_set8)
+static struct btf_id *add_set(struct object *obj, char *name, enum btf_id_kind kind)
{
+ int len = strlen(name);
+ int prefixlen;
+ char *id;
+
/*
* __BTF_ID__set__name
* name = ^
* id = ^
*/
- char *id = name + (is_set8 ? sizeof(BTF_SET8 "__") : sizeof(BTF_SET "__")) - 1;
- int len = strlen(name);
+ switch (kind) {
+ case BTF_ID_KIND_SET:
+ prefixlen = sizeof(BTF_SET "__") - 1;
+ break;
+ case BTF_ID_KIND_SET8:
+ prefixlen = sizeof(BTF_SET8 "__") - 1;
+ break;
+ default:
+ pr_err("Unexpected kind %d passed to %s() for symbol %s\n", kind, __func__, name);
+ return NULL;
+ }
+ id = name + prefixlen - 1;
if (id >= name + len) {
pr_err("FAILED to parse set name: %s\n", name);
return NULL;
}
- return btf_id__add(&obj->sets, id, true);
+ return btf_id__add(&obj->sets, id, kind);
}
static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
@@ -288,7 +312,7 @@ static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
return NULL;
}
- return btf_id__add(root, id, false);
+ return btf_id__add(root, id, BTF_ID_KIND_SYM);
}
/* Older libelf.h and glibc elf.h might not yet define the ELF compression types. */
@@ -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)
id->cnt = sym.st_size / sizeof(int) - 1;
- id->is_set = true;
- }
} else {
pr_err("FAILED unsupported prefix %s\n", prefix);
return -1;
@@ -643,7 +663,7 @@ static int id_patch(struct object *obj, struct btf_id *id)
int i;
/* For set, set8, id->id may be 0 */
- if (!id->id && !id->is_set && !id->is_set8) {
+ if (!id->id && id->kind != BTF_ID_KIND_SET && id->kind != BTF_ID_KIND_SET8) {
pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
warnings++;
}
@@ -696,6 +716,7 @@ static int sets_patch(struct object *obj)
{
Elf_Data *data = obj->efile.idlist;
struct rb_node *next;
+ int cnt;
next = rb_first(&obj->sets);
while (next) {
@@ -715,11 +736,15 @@ static int sets_patch(struct object *obj)
return -1;
}
- if (id->is_set) {
+ switch (id->kind) {
+ case BTF_ID_KIND_SET:
set = data->d_buf + off;
+ cnt = set->cnt;
qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
- } else {
+ break;
+ case BTF_ID_KIND_SET8:
set8 = data->d_buf + off;
+ cnt = set8->cnt;
/*
* Make sure id is at the beginning of the pairs
* struct, otherwise the below qsort would not work.
@@ -744,10 +769,13 @@ static int sets_patch(struct object *obj)
bswap_32(set8->pairs[i].flags);
}
}
+ break;
+ default:
+ pr_err("Unexpected btf_id_kind %d for set '%s'\n", id->kind, id->name);
+ return -1;
}
- pr_debug("sorting addr %5lu: cnt %6d [%s]\n",
- off, id->is_set ? set->cnt : set8->cnt, id->name);
+ pr_debug("sorting addr %5lu: cnt %6d [%s]\n", off, cnt, id->name);
next = rb_next(next);
}
--
2.52.0On 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.
> + 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).
> id->cnt = sym.st_size / sizeof(int) - 1;
> - id->is_set = true;
> - }
> } else {
> pr_err("FAILED unsupported prefix %s\n", prefix);
> return -1;
[...]
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;
>
> [...]
>
On Mon, 2025-12-15 at 18:31 -0800, Ihor Solodrai wrote:
> 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.
On the contrary, the callsite knows exactly what it wants:
unique or non-unique entries. Here you need additional logic
to figure out the intent.
Arguably the uniqueness is associated not with entry type,
but with a particular tree the entry is added to.
And that is a property of the callsite.
> > > + 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.
Oh, ok, sorry, didn't notice that.
>
> >
> > > id->cnt = sym.st_size / sizeof(int) - 1;
> > > - id->is_set = true;
> > > - }
> > > } else {
> > > pr_err("FAILED unsupported prefix %s\n", prefix);
> > > return -1;
> >
> > [...]
> >
On 12/15/25 6:38 PM, Eduard Zingerman wrote: > On Mon, 2025-12-15 at 18:31 -0800, Ihor Solodrai wrote: >> 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. > > On the contrary, the callsite knows exactly what it wants: > unique or non-unique entries. Here you need additional logic > to figure out the intent. > > Arguably the uniqueness is associated not with entry type, > but with a particular tree the entry is added to. > And that is a property of the callsite. You're right that the uniqueness is associated with a tree. This means we could even check the kind of the root... I'm thinking maybe it's cleaner to have btf_id__add() and btf_id__add_unique(). It can even be a wrapper around btf_id__add() with a boolean. wdyt? > >>> >>> [...] >>>
On Mon, 2025-12-15 at 18:52 -0800, Ihor Solodrai wrote: > On 12/15/25 6:38 PM, Eduard Zingerman wrote: > > On Mon, 2025-12-15 at 18:31 -0800, Ihor Solodrai wrote: > > > 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. > > > > On the contrary, the callsite knows exactly what it wants: > > unique or non-unique entries. Here you need additional logic > > to figure out the intent. > > > > Arguably the uniqueness is associated not with entry type, > > but with a particular tree the entry is added to. > > And that is a property of the callsite. > > You're right that the uniqueness is associated with a tree. > This means we could even check the kind of the root... > > I'm thinking maybe it's cleaner to have btf_id__add() and > btf_id__add_unique(). It can even be a wrapper around btf_id__add() > with a boolean. wdyt? Well, sure, that would be a bit cleaner on the callsite. Up to you, given the number of the callsites I wouldn't bother.
© 2016 - 2025 Red Hat, Inc.