Re: [PATCH bpf-next v2 1/4] libbpf: put forward declarations to btf_dump->emit_queue

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2024-05-28 at 15:05 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -93,7 +85,10 @@ struct btf_dump {
> >         size_t cached_names_cap;
> > 
> >         /* topo-sorted list of dependent type definitions */
> > -       __u32 *emit_queue;
> > +       struct {
> > +               __u32 id:31;
> > +               __u32 fwd:1;
> > +       } *emit_queue;
> 
> let's define the named type right in this patch, no need to use
> typeof() hack just to remove it later.
> 
> Also, let's maybe have
> 
> struct <whatever> {
>     __u32 id;
>     __u32 flags;
> };
> 
> and define
> 
> enum btf_dump_emit_flags {
>     BTF_DUMP_FWD_DECL = 0x1,
> };
> 
> Or something along those lines? Having a few more flags available will
> make it less like that we'll need to add a new set of APIs just to
> accommodate one extra flag. (Though, if we add another field, we'll
> end up adding another API anyways, but I really hope we will never
> have to do this).

Ok, will do

[...]

> > -static int btf_dump_add_emit_queue_id(struct btf_dump *d, __u32 id)
> > +static int __btf_dump_add_emit_queue_id(struct btf_dump *d, __u32 id, bool fwd)
> 
> I don't like those underscored functions in libbpf code base, please
> don't add them. But I'm also not sure we need to have it, there are
> only a few calles of the original btf_dump_add_emit_queue_id(), so we
> can just update them to pass true/false as appropriate.

Will do

[...]

> > +static bool btf_dump_emit_missing_aliases(struct btf_dump *d, __u32 id, bool dry_run);
> > +
> > +static bool btf_dump_is_blacklisted(struct btf_dump *d, __u32 id)
> > +{
> > +       const struct btf_type *t = btf__type_by_id(d->btf, id);
> > +
> > +       /* __builtin_va_list is a compiler built-in, which causes compilation
> > +        * errors, when compiling w/ different compiler, then used to compile
> > +        * original code (e.g., GCC to compile kernel, Clang to use generated
> > +        * C header from BTF). As it is built-in, it should be already defined
> > +        * properly internally in compiler.
> > +        */
> > +       if (t->name_off == 0)
> > +               return false;
> > +       return strcmp(btf_name_of(d, t->name_off), "__builtin_va_list") == 0;
> > +}
> > +
> 
> why moving btf_dump_is_blacklisted() but forward declaring
> btf_dump_emit_missing_aliases()? Let's do the same to both, whichever
> it is (forward declaring probably is least distracting here)

Sure, will add forward declarations for both.

[...]

> >         case BTF_KIND_UNION: {
> >                 const struct btf_member *m = btf_members(t);
> > +               __u32 new_cont_id;
> > +
> >                 /*
> >                  * struct/union is part of strong link, only if it's embedded
> >                  * (so no ptr in a path) or it's anonymous (so has to be
> >                  * defined inline, even if declared through ptr)
> >                  */
> > -               if (through_ptr && t->name_off != 0)
> > -                       return 0;
> > +               if (through_ptr && t->name_off != 0) {
> > +                       if (id != cont_id)
> > +                               return btf_dump_add_emit_queue_fwd(d, id);
> > +                       else
> > +                               return 0;
> 
> very subjective nit, but this "else return 0;" just doesn't feel right
> here. Let's do:
> 
> if (id == cont_id)
>     return 0;
> return btf_dump_add_emit_queue_fwd();
> 
> It feels a bit more natural as "if it's a special nice case, we are
> done (return 0); otherwise we need to emit extra fwd decl."

Will do

> 
> > +               }
> > 
> >                 tstate->order_state = ORDERING;
> > 
> > +               new_cont_id = t->name_off == 0 ? cont_id : id;
> >                 vlen = btf_vlen(t);
> >                 for (i = 0; i < vlen; i++, m++) {
> > -                       err = btf_dump_order_type(d, m->type, false);
> > +                       err = btf_dump_order_type(d, m->type, new_cont_id, false);
> 
> just inline `t->name_off ? id : cont_id` here? It's short and
> straightforward enough, I suppose (named type defines new containing
> "scope", anonymous type continues existing scope)

Will do

[...]

> > +               err = btf_dump_add_emit_queue_fwd(d, id);
> > +               if (err)
> > +                       return err;
> > +               return 0;
> 
> return btf_dump_add_emit_queue_fwd(...); ? this is the last step, so
> seems appropriate

Will do

> >         case BTF_KIND_VOLATILE:
> >         case BTF_KIND_CONST:
> >         case BTF_KIND_RESTRICT:
> >         case BTF_KIND_TYPE_TAG:
> > -               return btf_dump_order_type(d, t->type, through_ptr);
> > +               return btf_dump_order_type(d, t->type, cont_id, through_ptr);
> > 
> >         case BTF_KIND_FUNC_PROTO: {
> >                 const struct btf_param *p = btf_params(t);
> > -               bool is_strong;
> > 
> > -               err = btf_dump_order_type(d, t->type, through_ptr);
> > +               err = btf_dump_order_type(d, t->type, cont_id, through_ptr);
> >                 if (err < 0)
> >                         return err;
> > -               is_strong = err > 0;
> > 
> >                 vlen = btf_vlen(t);
> >                 for (i = 0; i < vlen; i++, p++) {
> > -                       err = btf_dump_order_type(d, p->type, through_ptr);
> > +                       err = btf_dump_order_type(d, p->type, cont_id, through_ptr);
> >                         if (err < 0)
> >                                 return err;
> > -                       if (err > 0)
> > -                               is_strong = true;
> >                 }
> > -               return is_strong;
> > +               return err;
> 
> this should always be zero, right? Just return zero explicit, don't
> make reader to guess

Ok

[...]

> > @@ -1037,19 +1006,21 @@ static const char *missing_base_types[][2] = {
> >         { "__Poly128_t",        "unsigned __int128" },
> >  };
> > 
> > -static void btf_dump_emit_missing_aliases(struct btf_dump *d, __u32 id,
> > -                                         const struct btf_type *t)
> > +static bool btf_dump_emit_missing_aliases(struct btf_dump *d, __u32 id, bool dry_run)
> 
> this dry_run approach look like a sloppy hack, tbh. Maybe just return
> `const char *` of aliased name, and let caller handle whatever is
> necessary (either making decision about the need for alias or actually
> emitting `typedef %s %s`?

I had a version w/o dry run but it seemed to heavy-handed for the purpose:

static int btf_dump_missing_alias_idx(struct btf_dump *d, __u32 id)
{
	const char *name = btf_dump_type_name(d, id);
	int i;

	for (i = 0; i < ARRAY_SIZE(missing_base_types); i++) {
		if (strcmp(name, missing_base_types[i][0]) == 0)
			return i;
	}

        return -1;
}

static bool btf_dump_is_missing_alias(struct btf_dump *d, __u32 id)
{
	return btf_dump_missing_alias_idx(d, id) >= 0;
}

static bool btf_dump_emit_missing_aliases(struct btf_dump *d, __u32 id)
{
	const char *name = btf_dump_type_name(d, id);
	int i;

	i = btf_dump_missing_alias_idx(d, id);
	if (i < 0)
		return false;

	btf_dump_printf(d, "typedef %s %s;\n\n",
			missing_base_types[i][1], name);
	return true;
}

I can use the above if you think it is better.