Re: [PATCH bpf-next 1/2] libbpf: accommodate DWARF/compiler bug with duplicated structs

[Jiri Olsa <jolsa@redhat.com>]

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@kernel.org> 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@kernel.org>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  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
> >
>