On Mon, Mar 14, 2022 at 4:18 PM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> Thanks, Andrii! The nits/refactors are straightforward but have some questions
> below:
>
> On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote:
> > > +
> > > + /* sanitize variable name, e.g., for static vars inside
> > > + * a function, it's name is '<function name>.<variable name>',
> > > + * which we'll turn into a '<function name>_<variable name>'.
> > > + */
> > > + sanitize_identifier(var_ident + 1);
> >
> > btw, I think we don't need sanitization anymore. We needed it for
> > static variables (they would be of the form <func_name>.<var_name> for
> > static variables inside the functions), but now it's just unnecessary
> > complication
>
> How would we handle static variables inside functions in libraries then?
That's the thing, we don't expose them (anymore) in skeletons. So we
skip anything static. Anything global should have a unique and valid C
identifier name.
>
> >
> > > + var_ident[0] = ' ';
> > > +
> > > + /* The datasec member has KIND_VAR but we want the
> > > + * underlying type of the variable (e.g. KIND_INT).
> > > + */
> > > + var = btf__type_by_id(btf, var->type);
> >
> > you need to use skip_mods_and_typedefs() or equivalent to skip any
> > const/volatile/restrict modifiers before checking btf_is_array()
>
> Good catch!
>
> >
> > > +
> > > + /* Prepend * to the field name to make it a pointer. */
> > > + var_ident[0] = '*';
> > > +
> > > + printf("\t\t");
> > > + /* Func and array members require special handling.
> > > + * Instead of producing `typename *var`, they produce
> > > + * `typeof(typename) *var`. This allows us to keep a
> > > + * similar syntax where the identifier is just prefixed
> > > + * by *, allowing us to ignore C declaration minutae.
> > > + */
> > > + if (btf_is_array(var) ||
> > > + btf_is_ptr_to_func_proto(btf, var)) {
> > > + printf("typeof(");
> > > + /* print the type inside typeof() without a name */
> > > + opts.field_name = "";
> > > + err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> > > + if (err)
> > > + goto out;
> > > + printf(") %s", var_ident);
> > > + } else {
> > > + err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> > > + if (err)
> > > + goto out;
> > > + }
> > > + printf(";\n");
> >
> > we can simplify this a bit around var_ident and two
> > btf_dump__emit_type_decl() invocations. We know that we are handling
> > "non-uniform" C syntax for array and func pointer, so we don't need to
> > use opts.field_name. Doing this (schematically) should work (taking
> > into account no need for sanitization as well):
> >
> > if (btf_is_array() || btf_is_ptr_to_func_proto())
> > printf("typeof(");
> > btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
> > printf(" *%s", var_name);
> >
> > or did I miss some corner case?
>
> You didn't close the "typeof" :)
Eagle eye :)
>
> if (btf_is_array() || btf_is_ptr_to_func_proto())
> printf("typeof(");
> btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
> if (btf_is_array() || btf_is_ptr_to_func_proto())
> printf(")");
> printf(" *%s", var_name);
>
> If you feel that's easier to understand, sure. I don't love it but it's
> understandable enough.
all the string buffer manipulations seem worse (you can also have a
variable to record the decision whether to use typeof or not, so you
don't have to repeat verbose is_array || is_ptr_to_func_proto check)
>
> [...]
>
>
> > we don't know the name of the final object, why would we allow to set
> > any object name at all?
>
> We don't really care about the final object name but we do need an object name
> for the subskeleton. The subskeleton type name, header guard etc all use it.
> We can say that it's always taken from the file name, but giving the user the
> option to override it feels right, given the parallel with skeletons (and what
> would we do if the file name is a pipe from a subshell invocation?).
Ah, I see, it's sort of like "library name" in this case. Yeah, makes
sense, missed that part. I was too much focused on not letting it get
into map names :)
>
> > >
> > > +
> > > + /* The empty object name allows us to use bpf_map__name and produce
> > > + * ELF section names out of it. (".data" instead of "obj.data")
> > > + */
> > > + opts.object_name = "";
> >
> > yep, like this. So that "name" argument "support" above is bogus,
> > let's remove it
>
> See above, it changes real things.
yep, my bad
>
> >
> > > + obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> > > + if (!obj) {
> > > + char err_buf[256];
> > > +
> > > + libbpf_strerror(errno, err_buf, sizeof(err_buf));
> > > + p_err("failed to open BPF object file: %s", err_buf);
> > > + obj = NULL;
> > > + goto out;
> > > + }
> > > +
> >
> > [...]
> >
> > > + for (i = 0; i < len; i++, var++) {
> > > + var_type = btf__type_by_id(btf, var->type);
> > > + var_name = btf__name_by_offset(btf, var_type->name_off);
> > > +
> > > + if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
> > > + continue;
> > > +
> > > + var_ident[0] = '\0';
> > > + strncat(var_ident, var_name, sizeof(var_ident) - 1);
> > > + sanitize_identifier(var_ident);
> > > +
> > > + /* Note that we use the dot prefix in .data as the
> > > + * field access operator i.e. maps%s becomes maps.data
> > > + */
> > > + codegen("\
> > > + \n\
> > > + s->vars[%4$d].name = \"%1$s\"; \n\
> > > + s->vars[%4$d].map = &obj->maps%3$s; \n\
> > > + s->vars[%4$d].addr = (void**) &obj->%2$s.%1$s;\n\
> > > + ", var_ident, ident, bpf_map__name(map), var_idx);
> >
> > map reference should be using ident, not bpf_map__name(), as it refers
> > to a field. The way it is now it shouldn't work for custom
> > .data.my_section case (do you have a test for this?) You shouldn't
> > need bpf_map__name() here at all.
>
> Good catch, I'll add a .data.custom test.
>
> [...]
>
> >
> > > + " %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n"
> >
> > [name OBJECT_NAME] should be supported
>
> Not sure what you mean by "supported" here.
"not" was missing, but we just concluded that it is indeed needed
>
> >
> > > " %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
> > > " %1$s %2$s help\n"
> > > "\n"
> > > @@ -1788,6 +2250,7 @@ static int do_min_core_btf(int argc, char **argv)
> > > static const struct cmd cmds[] = {
> > > { "object", do_object },
> > > { "skeleton", do_skeleton },
> > > + { "subskeleton", do_subskeleton },
> > > { "min_core_btf", do_min_core_btf},
> > > { "help", do_help },
> > > { 0 }
> > > --
> > > 2.34.1
>
> -- Delyan
>
