On Mon, Apr 28, 2025 at 5:33 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Mon, Apr 28, 2025 at 3:12 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Mon, Apr 28, 2025 at 8:21 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> > >
> > > <1><4bd05>: Abbrev Number: 58 (DW_TAG_pointer_type)
> > > <4bd06> DW_AT_byte_size : 8
> > > <4bd07> DW_AT_address_class: 2
> > > <4bd08> DW_AT_type : <0x301cd>
> > >
> > > ...which points at an int
> > >
> > > <1><301cd>: Abbrev Number: 214 (DW_TAG_base_type)
> > > <301cf> DW_AT_byte_size : 4
> > > <301d0> DW_AT_encoding : 5 (signed)
> > > <301d1> DW_AT_name : int
> > > <301d5> DW_AT_name : int
> > >
> > > ...but note the the DW_AT_address_class attribute in the latter case and
> > > the two DW_AT_name values. We don't use that address attribute in pahole
> > > as far as I can see, but it might be enough to cause problems.
> >
> > DW_AT_address_class is there because it's an actual address space
> > qualifier in C. The dwarf is correct, but I thought pahole
> > will ignore it while converting to BTF, so it shouldn't matter
> > from dedup pov.
> >
> > And since dedup is working for vmlinux BTF, I doubt there are CUs
> > where the same type is represented with different dwarf id-s.
> > Otherwise dedup wouldn't have worked for vmlinux.
> >
> > DW_AT_name is concerning. Sounds like it's a gcc bug, but it
> > shouldn't be causing dedup issues for modules.
> >
> > So what is the workaround?
>
> I'm thinking of generalizing Alan's proposed fix so that all our
> existing special equality cases (arrays, identical structs, and now
> pointers to identical types) are handled a bit more generically. I'll
> try to get a patch out later tonight.
So I ran out of time, but I'm thinking something like below. It
results in identical bpf_testmod.ko compared to Alan's proposed fix,
so perhaps we should just go with the simpler approach. But this one
should stand the test of time a bit better.
In any case, I'd like to be able to handle not just PTR -> TYPE chain,
but also some PTR -> MODIFIER* -> TYPE chains at the very least.
Because any const in the chain will throw off Alan's heuristic.
I'll try to benchmark and polish the patch tomorrow and post it for
proper review.
diff --git a/src/btf.c b/src/btf.c
index e9673c0ecbe7..e4a3e3183742 100644
--- a/src/btf.c
+++ b/src/btf.c
@@ -4310,6 +4310,8 @@ static bool btf_dedup_identical_arrays(struct
btf_dedup *d, __u32 id1, __u32 id2
return btf_equal_array(t1, t2);
}
+static bool btf_dedup_identical_types(struct btf_dedup *d, __u32 id1,
__u32 id2);
+
/* Check if given two types are identical STRUCT/UNION definitions */
static bool btf_dedup_identical_structs(struct btf_dedup *d, __u32
id1, __u32 id2)
{
@@ -4329,14 +4331,93 @@ static bool btf_dedup_identical_structs(struct
btf_dedup *d, __u32 id1, __u32 id
m1 = btf_members(t1);
m2 = btf_members(t2);
for (i = 0, n = btf_vlen(t1); i < n; i++, m1++, m2++) {
- if (m1->type != m2->type &&
- !btf_dedup_identical_arrays(d, m1->type, m2->type) &&
- !btf_dedup_identical_structs(d, m1->type, m2->type))
+ if (m1->type != m2->type && !btf_dedup_identical_types(d, m1->type, m2->type))
+ return false;
+ }
+ return true;
+}
+
+static bool btf_dedup_identical_fnprotos(struct btf_dedup *d, __u32
id1, __u32 id2)
+{
+ const struct btf_param *p1, *p2;
+ struct btf_type *t1, *t2;
+ int n, i;
+
+ t1 = btf_type_by_id(d->btf, id1);
+ t2 = btf_type_by_id(d->btf, id2);
+
+ if (!btf_is_func_proto(t1) || !btf_is_func_proto(t2))
+ return false;
+
+ if (!btf_compat_fnproto(t1, t2))
+ return false;
+
+ if (!btf_dedup_identical_types(d, t1->type, t2->type))
+ return false;
+
+ p1 = btf_params(t1);
+ p2 = btf_params(t2);
+ for (i = 0, n = btf_vlen(t1); i < n; i++, p1++, p2++) {
+ if (p1->type != p2->type && !btf_dedup_identical_types(d, p1->type, p2->type))
return false;
}
return true;
}
+static bool btf_dedup_identical_types(struct btf_dedup *d, __u32 id1,
__u32 id2)
+{
+ int max_depth = 32;
+
+ while (max_depth-- > 0) {
+ struct btf_type *t1, *t2;
+ int k1, k2;
+
+ t1 = btf_type_by_id(d->btf, id1);
+ t2 = btf_type_by_id(d->btf, id2);
+
+ k1 = btf_kind(t1);
+ k2 = btf_kind(t2);
+ if (k1 != k2)
+ return false;
+
+ switch (k1) {
+ case BTF_KIND_UNKN: /* VOID */
+ return true;
+ case BTF_KIND_INT:
+ return btf_equal_int_tag(t1, t2);
+ case BTF_KIND_ENUM:
+ case BTF_KIND_ENUM64:
+ return btf_compat_enum(t1, t2);
+ case BTF_KIND_FWD:
+ case BTF_KIND_FLOAT:
+ return btf_equal_common(t1, t2);
+ case BTF_KIND_CONST:
+ case BTF_KIND_VOLATILE:
+ case BTF_KIND_RESTRICT:
+ case BTF_KIND_PTR:
+ case BTF_KIND_TYPEDEF:
+ case BTF_KIND_FUNC:
+ case BTF_KIND_TYPE_TAG:
+ if (t1->info != t2->info)
+ return 0;
+ id1 = t1->type;
+ id2 = t2->type;
+ continue;
+ case BTF_KIND_ARRAY:
+ return btf_equal_array(t1, t2);
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION:
+ return btf_dedup_identical_structs(d, id1, id2);
+ case BTF_KIND_FUNC_PROTO:
+ return btf_dedup_identical_fnprotos(d, id1, id2);
+ default:
+ return false;
+ }
+ }
+ return false;
+}
+
+
/*
* Check equivalence of BTF type graph formed by candidate struct/union (we'll
* call it "candidate graph" in this description for brevity) to a type graph
@@ -4458,8 +4539,6 @@ static int btf_dedup_is_equiv(struct btf_dedup
*d, __u32 cand_id,
* types within a single CU. So work around that by explicitly
* allowing identical array types here.
*/
- if (btf_dedup_identical_arrays(d, hypot_type_id, cand_id))
- return 1;
/* It turns out that similar situation can happen with
* struct/union sometimes, sigh... Handle the case where
* structs/unions are exactly the same, down to the referenced
@@ -4467,7 +4546,7 @@ static int btf_dedup_is_equiv(struct btf_dedup
*d, __u32 cand_id,
* types are different, but equivalent) is *way more*
* complicated and requires a many-to-many equivalence mapping.
*/
- if (btf_dedup_identical_structs(d, hypot_type_id, cand_id))
+ if (btf_dedup_identical_types(d, hypot_type_id, cand_id))
return 1;
return 0;
}
>
> >
> > We need to find it asap. Since at present we cannot build
> > kernels with gcc-14, since modules won't dedup BTF.
> > Hence a bunch of selftests/bpf are failing.
> > We want to upgrade BPF CI to gcc-14 to catch nginx-like issues,
> > but we cannot until this pahole/dedup issue is resolved.
