On 9/24/20 11:21 PM, Andrii Nakryiko wrote:
On Thu, Sep 24, 2020 at 8:55 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
On Wed, Sep 23, 2020 at 08:54:34AM -0700, Andrii Nakryiko wrote:
Add APIs for appending new BTF types at the end of BTF object.
Each BTF kind has either one API of the form btf__append_<kind>(). For types
that have variable amount of additional items (struct/union, enum, func_proto,
datasec), additional API is provided to emit each such item. E.g., for
emitting a struct, one would use the following sequence of API calls:
btf__append_struct(...);
btf__append_field(...);
...
btf__append_field(...);
I've just started looking through the diffs. The first thing that struck me
is the name :) Why 'append' instead of 'add' ? The latter is shorter.
Append is very precise about those types being added at the end. Add
is more ambiguous in that sense and doesn't imply any specific order.
E.g., for btf__add_str() that's suitable, because the order in which
strings are inserted might be different (e.g., if the string is
duplicated). But it's not an "insert" either, so I'm fine with
renaming to "add", if you prefer it.
The reason I prefer shorter is to be able to write:
btf__add_var(btf, "my_var", global,
btf__add_const(btf,
btf__add_volatile(btf,
btf__add_ptr(btf,
btf__add_int(btf, "int", 4, signed))));
In other words the shorter the type the more suitable the api
will be for manual construction of types.
Looks like the api already checks for invalid type_id,
so no need to check validity at every build stage.
Hence it's nice to combine multiple btf__add_*() into single line.
I think C language isn't great for 'constructor' style api.
May be on top of the above api we can add c++-like api?
For example we can define
struct btf_builder {
struct btf_builder * (*_volatile) (void);
struct btf_builder * (*_const) (void);
struct btf_builder * (*_int) (char *name, int sz, int encoding);
struct btf_builder * (_ptr) (void);
};
and the use it as:
struct btf_builder *b = btf__create_global_builer(...);
b->_int("int", 4, singed)
->_const()
->_volatile()
->_ptr()
->_var("my_var", global);
Every method will be return its own object (only one such object)
while the actual building will be happening in some 'invisible',
global, and mutex protected place.
Also how would you add anon struct that is within another struct ?
The anon one would have to be added first and then added as a field?
Feels a bit odd that struct/union building doesn't have 'finish' method,
but I guess it can work.
That embedded anon struct will be a separate type, then the field
(anonymous or not, that's orthogonal to anonymity of a struct (!))
will refer to that anon struct type by its ID. Anon struct can be
added right before, right after, or in between completely unrelated
types, it doesn't matter to BTF itself as long as all the type IDs
match in the end.
As for the finish method... There wasn't a need so far to have it, as
BTF constantly maintains correct vlen for the "current"
struct/union/func_proto/datasec/enum (anything with extra members).
I've been writing a few more tests than what I posted here (they will
come in the second wave of patches) and converted pahole to this new
API completely. And it does feel pretty nice and natural so far. In
the future we might add something like that, I suppose, that would
allow to do some more validations at the end. But that would be a
non-breaking extension, so I don't want to do it right now.
sure. that's fine.
Also I suspect sooner or later would be good to do at least partial
dedup of types while they're being built.
Instead of passing exact btf_id of 'int' everywhere it would be
human friendlier to say:
b->_int("int", 4, singed)->_var("my_var")
instead of having extra variable that holds btf_id of 'int' and
use as it:
u32 btf_id_of_int; /* that is used everywhere where 'int' field of
var needs to be added */
b->__var("my_var", btf_id_of_int);
I mean since types are being built the real dedup cannot happen,
but dedup of simple types can happen on the fly.
That will justify 'add' vs 'append' as well.
Just a thought.
