On Wed, 2024-01-03 at 15:59 -0800, Andrii Nakryiko wrote:
[...]
> > > > > + fn_id = btf__add_func(btf, prog->name, btf_func_linkage(fn_t), fn_t->type);
> > > >
> > > > Nit: Why not call this function near the end, when fn_proto_id is available?
> > > > Thus avoiding need to "guess" fn_t->type.
> > > >
> > >
> > > I think I did it to not have to remember btf_func_linkage(fn_t)
> > > (because fn_t will be invalidated) and because name_off will be reused
> > > for parameters. Neither is a big deal, I'll adjust to your suggestion.
> > >
> > > But note, we are not guessing ID, it's guaranteed to be +1, it's an
> > > API contract of btf__add_xxx() APIs.
> >
> > Noted, well, maybe just skip this nit in such a case.
> >
>
> I already did the change locally, as I said it's a small change, so no problem.
Oh, ok, thanks.
[...]
> > > > > + /* clone fn/fn_proto, unless we already did it for another arg */
> > > > > + if (func_rec->type_id == orig_fn_id) {
> > > > > + int fn_id;
> > > > > +
> > > > > + fn_id = clone_func_btf_info(btf, orig_fn_id, prog);
> > > > > + if (fn_id < 0) {
> > > > > + err = fn_id;
> > > > > + goto err_out;
> > > > > + }
> > > > > +
> > > > > + /* point func_info record to a cloned FUNC type */
> > > > > + func_rec->type_id = fn_id;
> > > >
> > > > Would it be helpful to add a log here, saying that BTF for function
> > > > so and so is changed before load?
> > >
> > > Would it? We don't have global subprog's name readily available, it
> > > seems. So I'd need to refetch it by fn_id, then btf__str_by_offset()
> > > just to emit cryptic (for most users) notifications that something
> > > about some func info was adjusted. And then the user would get this
> > > same message for the same subprog but in the context of a different
> > > entry program. Just confusing, tbh.
> > >
> > > Unless you insist, I'd leave it as is. This logic is supposed to be
> > > bullet-proof, so I'm not worried about debugging regressions with it
> > > (but maybe I'm delusional).
> >
> > I was thinking about someone finding out that actual in-kernel BTF
> > is different from that in the program object file, while debugging
> > something. Might be a bit surprising. I'm not insisting on this, though.
>
> Note the "/* check if existing parameter already matches verifier
> expectations */", if program is using correct types, we don't touch
> BTF for that subprog. If there was `void *ctx`, we don't really lose
> any information.
But `void *ctx` would be changed to `struct bpf_user_pt_regs_t *ctx`, right?
And that might be a bit surprising. But whatever, if you think that adding
log entry here is too much of hassle -- let's leave it as is.
> If they use `struct pt_regs *ctx __arg_ctx`, then yeah, it will be
> updated to `struct bpf_user_pt_regs_t *ctx __arg_ctx`, but even then,
> original BTF has original FUNC -> FUNC_PROTO definition. You'd need to
> fetch func_info and follow BTF IDs (I'm not sure if bpftool even shows
> this today).
>
> In short, I don't see why this would be a problem, but perhaps I
> should just bite a bullet and do feature detector for this support.
I like that current implementation does the transformation unconditionally,
it does no harm and avoids unnecessary branching.
[...]
