On Tue, May 28, 2024 at 3:25 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
I meant something less heavy-handed:
static const char *btf_dump_missing_alias(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 missing_base_types[i][1];
}
return NULL;
}
And we actually don't need to use btf_dump_type_name(), btf_name_of()
should be more than adequate for this.
Then if you get NULL from this function, there is no aliasing
required. If you got non-NULL, you have the name you should alias to
(btf_name_of() will give you original name).
