On Mon, Apr 28, 2025 at 11:59 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
>
Ok, so sleeping on this a bit more, I'm hesitant to do this more
generic approach, as now we'll be running a risk of potentially
looping indefinitely (the max_depth check I added doesn't completely
prevent this).
So, Alan, do you mind sending your proposed patch for formal review
and BPF CI testing? Just please use btf_is_ptr() check instead of
explicit kind equality, thanks!
> 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.
