On 2/1/24 16:51, Daniel Borkmann wrote:
> On 1/31/24 5:24 PM, Viktor Malik wrote:
>> Instead of using magic offsets to access BTF ID set data, leverage types
>> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
>> layout of the data. Thanks to this change, set sorting should also
>> continue working if the layout changes.
>>
>> This requires to sync the definition of 'struct btf_id_set8' from
>> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
>> the rest of the file at the moment, b/c that would require to also sync
>> multiple dependent headers and we don't need any other defs from
>> btf_ids.h.
>>
>> Signed-off-by: Viktor Malik <vmalik@xxxxxxxxxx>
>> ---
>> tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>> tools/include/linux/btf_ids.h | 9 +++++++++
>> 2 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 27a23196d58e..7badf1557e5c 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -70,6 +70,7 @@
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <errno.h>
>> +#include <linux/btf_ids.h>
>> #include <linux/rbtree.h>
>> #include <linux/zalloc.h>
>> #include <linux/err.h>
>> @@ -78,7 +79,7 @@
>> #include <subcmd/parse-options.h>
>>
>> #define BTF_IDS_SECTION ".BTF_ids"
>> -#define BTF_ID "__BTF_ID__"
>> +#define BTF_ID_PREFIX "__BTF_ID__"
>>
>> #define BTF_STRUCT "struct"
>> #define BTF_UNION "union"
>> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>>
>> static bool is_btf_id(const char *name)
>> {
>> - return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
>> + return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>> }
>>
>> static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
>> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>> * __BTF_ID__TYPE__vfs_truncate__0
>> * prefix = ^
>> */
>> - prefix = name + sizeof(BTF_ID) - 1;
>> + prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>>
>> /* struct */
>> if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
>> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>> while (next) {
>> unsigned long addr, idx;
>> struct btf_id *id;
>> - int *base;
>> - int cnt;
>> + void *base;
>> + int cnt, size;
>>
>> id = rb_entry(next, struct btf_id, rb_node);
>> addr = id->addr[0];
>> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>> }
>>
>> idx = idx / sizeof(int);
>> - base = &ptr[idx] + (id->is_set8 ? 2 : 1);
>> - cnt = ptr[idx];
>> + if (id->is_set) {
>> + struct btf_id_set *set;
>> +
>> + set = (struct btf_id_set *)&ptr[idx];
>> + base = set->ids;
>> + cnt = set->cnt;
>> + size = sizeof(set->ids[0]);
>> + } else {
>> + struct btf_id_set8 *set8;
>> +
>> + set8 = (struct btf_id_set8 *)&ptr[idx];
>> + base = set8->pairs;
>> + cnt = set8->cnt;
>> + size = sizeof(set8->pairs[0]);
>> + }
>>
>> pr_debug("sorting addr %5lu: cnt %6d [%s]\n",
>> (idx + 1) * sizeof(int), cnt, id->name);
>>
>> - qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>> + qsort(base, cnt, size, cmp_id);
>
> Looks good to me, one small remark: perhaps it would also make sense to have an assert
> on the id location, such that we have a build error in case the id would not be the
> first in the struct / pairs array anymore given then cmp_id would look at wrong data
> for the latter given the plain int cast from there.
That's a good idea, I'll add a BUILD_BUG_ON check for set8.
Thanks!
>
>> next = rb_next(next);
>> }
>> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
>> index 2f882d5cb30f..72535f00572f 100644
>> --- a/tools/include/linux/btf_ids.h
>> +++ b/tools/include/linux/btf_ids.h
>> @@ -8,6 +8,15 @@ struct btf_id_set {
>> u32 ids[];
>> };
>>
>> +struct btf_id_set8 {
>> + u32 cnt;
>> + u32 flags;
>> + struct {
>> + u32 id;
>> + u32 flags;
>> + } pairs[];
>> +};
>> +
>> #ifdef CONFIG_DEBUG_INFO_BTF
>>
>> #include <linux/compiler.h> /* for __PASTE */
>>
>
