On Wed, 20 May 2026 00:26:40 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> > Hmm, it may be better to make it one parenthesis?
> >
> > (STRUCT,PTR,ASSIGN)->FIELD
> >
> > data=(foo,foo_list,list)->data
>
> OK, but I don't like this 3 parameters conversion. I want to
> make it a kind of type casting with an option.
>
> (STRUCT,ASSIGN)PTR->FIELD
>
> data=(foo,list)foo_list->data
>
> The second parenthesis will be eventually needed for nested casting,
> for example, in above case, if the data is a pointer to another data
> structure:
>
> struct bar {
> int value;
> ...
> };
>
> value=(bar)((foo,list)foo_list->data)->value
Have fun with the parenthesis parsing ;-)
>
>
> >
> > That would make it easier to differentiate between a simple "typecast" and
> > a container_of() by checking if the content between the parenthesis has a
> > comma.
> >
> > Maybe even reorder it to:
> >
> > (PTR,STRUCT,ASSIGN)->FIELD
> >
> > data=(foo_list,foo,list)->data
> >
> > to match the order of container_of():
> >
> > data = container_of(foo_list, struct foo, list)->data;
> >
> > ?
>
> This doesn't seem to conform to the rule here of using parentheses for
> type casting, so I personally don't like it.
OK, as long as it's intuitive and is easy to remember. I hate having to
look up the documents every time I have to do some probe argument
parsing :-(
>
>
> > > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > > > index e0d3a0da26af..b0829eb1cb52 100644
> > > > --- a/kernel/trace/trace_probe.c
> > > > +++ b/kernel/trace/trace_probe.c
> > > > @@ -464,6 +464,26 @@ static const char *fetch_type_from_btf_type(struct
> > > > btf *btf,
> > > > return NULL;
> > > > }
> > > >
> > > > +static int query_btf_struct(const char *sname, struct
> > > > traceprobe_parse_context *ctx)
> > > > +{
> > > > + int id;
> > > > +
> > > > + if (!ctx->btf) {
> > > > + struct btf *btf;
> > >
> > > This needs an empty line here.
> >
> > Sure.
> >
> > For conditional blocks, I don't always add a newline, but this is your code
> > and I'll follow your suggestions.
>
> Ah, this is just for fixing checkpatch.pl warning :-)
I added it to keep your checkpatch happy.
> > > > @@ -471,12 +491,12 @@ static int query_btf_context(struct
> > > > traceprobe_parse_context *ctx)
> > > > struct btf *btf;
> > > > s32 nr;
> > > >
> > > > - if (ctx->btf)
> > > > - return 0;
> > > > -
> > > > if (!ctx->funcname)
> > > > return -EINVAL;
> > > >
> > > > + if (ctx->btf)
> > > > + return 0;
> > > > +
> > >
> > > Could you tell me why this order is changed?
> > > I think this type casting will allow us to skip checking funcname
> > > because btf context is already specified.
> >
> > I wanted this to fail if btf was already set but funcname wasn't, because
> > this should only be called for functions.
>
> Hmm, OK. Then, can you make a separate patch for this?
I added a struct_btf (I can change it to typecast_btf) and have a
helper function that is:
static struct btf *ctx_btf(struct traceprobe_parse_context *ctx)
{
return ctx->flags & TPARG_FL_STRUCT ?
ctx->struct_btf : ctx->btf;
}
And have all functions get their btf from that. This way we can keep
the two allocations separate.
>
> >
> > >
> > > Ah, BTW, we may need to use a special struct btf* for type
> > > casting. If the target function is in a module and the
> > > casting type is defined in vmlinux, those are stored in
> > > the different places...
> >
> > OK, I'll make a separate btf for it then. I'll have to make sure the btf
> > used for parsing knows which one to use. Shouldn't be too hard if we check
> > for the STRUCT flag in the ctx->flags.
>
> Yeah, and personally, I think that flag should be the TYPECAST flag.
I'll update it.
Thanks,
-- Steve
