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.
