[DTrace-devel] [PATCH 4/7] ctf: add BTF-to-CTF convertor

[Nick Alcock]

Reiterating what I said on IRC...

On 7 Nov 2023, Kris Van Hees stated:
>> > +		/* Do we already have this type? */
>> > +		switch (BTF_INFO_KIND(type->info)) {
>> > +		case BTF_KIND_ENUM:
>> > +		case BTF_KIND_ENUM64:
>> > +			if (snprintf(n, sizeof(n), "enum %s",
>> > +				     name == NULL ? "(anon)" : name) < 0)
>> > +				return dt_ctf_set_errno(dtp, ECTF_NAMELEN);
>> > +
>> > +			ctfid = ctf_lookup_by_name(ctf, n);
>> > +			break;
>> > +		case BTF_KIND_UNION:
>> > +			if (snprintf(n, sizeof(n), "union %s",
>> > +				     name == NULL ? "(anon)" : name) < 0)
>> > +				return dt_ctf_set_errno(dtp, ECTF_NAMELEN);
>> > +	
>> > +			ctfid = ctf_lookup_by_name(ctf, n);
>> > +			break;
>> > +		case BTF_KIND_STRUCT:
>> > +			if (snprintf(n, sizeof(n), "struct %s",
>> > +				     name == NULL ? "(anon)" : name) < 0)
>> > +				return dt_ctf_set_errno(dtp, ECTF_NAMELEN);
>> > +	
>> > +			ctfid = ctf_lookup_by_name(ctf, n);
>> 
>> Just in case, you probably want to make sure that the type kind of these
>> types is not CTF_K_FORWARD as well. (Forwards to enums/structs/unions
>> are in the enum/struct/union namespace and are returned when you look
>> those up by name: they are auto-promoted to the real type when you add
>> an actual struct/union/enum with the same name.)
>
> I don't think there is an issue with this.  Besides, it is possible that we
> refer to a struct or union at this point that is only known as a forward so
> it would be valid that it gets returned.  When we actually use it, it will
> get promoted to the real type anyway (if known), right?  So that still works.

True enough.

>> > +	case BTF_KIND_RESTRICT: {
>> > +		ctfid = dt_btf_add_to_ctf(dtp, btf, ctf, type->type);
>> > +		if (ctfid == CTF_ERR)
>> > +			return CTF_ERR;		/* errno already set */
>> > +
>> > +		ctfid = ctf_add_restrict(ctf, CTF_ADD_ROOT, ctfid);
>> > +		btf->ctfids[type_id] = ctfid;
>> > +
>> > +		return ctfid == CTF_ERR ? dt_ctf_error(dtp, ctf) : ctfid;
>> > +	}
>> 
>> It seems to me that all type chains (e.g. member foo -> const ->
>> restrict -> int) will end up duplicating all the unnamed (const,
>> restrict) components over and over again. Again, maybe this is harmless,
>> but it will lead to a *lot* of duplication (dwarf2ctf did this at one
>> point and we ended up with about 80% of our types being needlessly
>> duplicated unnamed intermediate things like cv-quals and pointers.)
>> 
>> But fixing this would probably make things much more complex: you'd need
>> to track the relationship between the type at the end of the chain and
>> all the types in all the chains that point to it, by which point you
>> probably want a hash-based deduplicator like the one in ctf-dedup, and
>> you should probably just be using CTF anyway :P
>> 
>> (So... probably this is right call? I doubt DTrace cares about the
>> duplication anyway.)
>
> It only increases the size of the CTF data by abuot 39kb so hardly something
> we worry about.

... and, as noted, uniquifying this stuff at insertion time is something
libctf should be doing, since such types are indistinguishable beyond ID
anyway. I've added this to my libctf todo list.

>> > +	if (ctf_update(ctf) == CTF_ERR)
>> > +		return NULL;
>> 
>> As a note, this does more or less nothing in modern CTF (but is
>> harmless). In particular it doesn't make access any faster or make it
>> 'like a ctf_open()ed dict' like old Solaris libctf used to (but it's
>> just a hash lookup anyway, so that doesn't matter).
>
> I'll admit I merely copied what DTrace was already doing.

It's harmless -- the only effect it has these days is to make
ctf_rollback work (rollbacks delete all types added since the last
update). It used to be a wildly expensive operation that reserialized
everything, and until you did it you couldn't read the stuff you'd just
added. (I purged all signs of this exponential-slowdown-inducing horror
in 2019.)

-- 
NULL && (void)