On Wed, Nov 17, 2021 at 11:42:58AM -0800, Andrii Nakryiko wrote:
> On Wed, Nov 17, 2021 at 11:41 AM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > According to [0], compilers sometimes might produce duplicate DWARF
> > definitions for exactly the same struct/union within the same
> > compilation unit (CU). We've had similar issues with identical arrays
> > and handled them with a similar workaround in 6b6e6b1d09aa ("libbpf:
> > Accomodate DWARF/compiler bug with duplicated identical arrays"). Do the
> > same for struct/union by ensuring that two structs/unions are exactly
> > the same, down to the integer values of field referenced type IDs.
>
> Jiri, can you please try this in your setup and see if that handles
> all situations or there are more complicated ones still. We'll need a
> test for more complicated ones in that case :( Thanks.
it seems to help largely, but I still see few modules (67 out of 780)
that keep 'struct module' for some reason.. their struct module looks
completely the same as is in vmlinux
I uploaded one of the module with vmlinux in here:
http://people.redhat.com/~jolsa/kmodbtf/2/
I will do some debugging and find out why it did not work in this module
and try to come up with another test
thanks,
jirka
>
> >
> > Solving this more generically (allowing referenced types to be
> > equivalent, but using different type IDs, all within a single CU)
> > requires a huge complexity increase to handle many-to-many mappings
> > between canonidal and candidate type graphs. Before we invest in that,
> > let's see if this approach handles all the instances of this issue in
> > practice. Thankfully it's pretty rare, it seems.
> >
> > [0] https://lore.kernel.org/bpf/YXr2NFlJTAhHdZqq@krava/
> >
> > Reported-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/btf.c | 45 +++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 41 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index b6be579e0dc6..e97217a77196 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -3477,8 +3477,8 @@ static long btf_hash_struct(struct btf_type *t)
> > }
> >
> > /*
> > - * Check structural compatibility of two FUNC_PROTOs, ignoring referenced type
> > - * IDs. This check is performed during type graph equivalence check and
> > + * Check structural compatibility of two STRUCTs/UNIONs, ignoring referenced
> > + * type IDs. This check is performed during type graph equivalence check and
> > * referenced types equivalence is checked separately.
> > */
> > static bool btf_shallow_equal_struct(struct btf_type *t1, struct btf_type *t2)
> > @@ -3851,6 +3851,31 @@ static int btf_dedup_identical_arrays(struct btf_dedup *d, __u32 id1, __u32 id2)
> > return btf_equal_array(t1, t2);
> > }
> >
> > +/* Check if given two types are identical STRUCT/UNION definitions */
> > +static bool btf_dedup_identical_structs(struct btf_dedup *d, __u32 id1, __u32 id2)
> > +{
> > + const struct btf_member *m1, *m2;
> > + 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_composite(t1) || btf_kind(t1) != btf_kind(t2))
> > + return false;
> > +
> > + if (!btf_shallow_equal_struct(t1, t2))
> > + return false;
> > +
> > + 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)
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > /*
> > * 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
> > @@ -3962,6 +3987,8 @@ static int btf_dedup_is_equiv(struct btf_dedup *d, __u32 cand_id,
> >
> > hypot_type_id = d->hypot_map[canon_id];
> > if (hypot_type_id <= BTF_MAX_NR_TYPES) {
> > + if (hypot_type_id == cand_id)
> > + return 1;
> > /* In some cases compiler will generate different DWARF types
> > * for *identical* array type definitions and use them for
> > * different fields within the *same* struct. This breaks type
> > @@ -3970,8 +3997,18 @@ 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.
> > */
> > - return hypot_type_id == cand_id ||
> > - btf_dedup_identical_arrays(d, hypot_type_id, cand_id);
> > + 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
> > + * type IDs. Anything more complicated (e.g., if referenced
> > + * 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))
> > + return 1;
> > + return 0;
> > }
> >
> > if (btf_dedup_hypot_map_add(d, canon_id, cand_id))
> > --
> > 2.30.2
> >
>
