lkml.org 
[lkml]   [2010]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 04/10][RFC] tracing: Remove per event trace registering
    * Steven Rostedt (rostedt@goodmis.org) wrote:
    > On Fri, 2010-04-30 at 15:06 -0400, Mathieu Desnoyers wrote:
    >
    > > > If it is possible sure, but that's the point. Where do you add the
    > > > check? The typecast is in the C code that is constant for all trace
    > > > events.
    > >
    > > You can add the call to the static inline type check directly within the
    > > generated probe function, right after the local variable declarations.
    >
    > Well, one thing, the callback is not going to be the same as the
    > DECLARE_TRACE() because the prototype ends with "void *data", and the
    > function being called actually uses the type of that data.
    >
    > We now will have:
    >
    > DEFINE_TRACE(mytracepoint, int myarg, myarg);
    >
    > void mycallback(int myarg, struct mystuct *mydata);
    >
    > register_trace_mytracepoint_data(mycallback, mydata)
    >
    > There's no place in DEFINE_TRACE to be able to test the type of data
    > that is being passed back. I could make the calling function be:
    >
    > void mycallback(int myarg, void *data)
    > {
    > struct mystruct *mydata = data;
    > [...]
    >
    > Because the data is defined uniquely by the caller that registers a
    > callback. Each function can register its own data type.

    Yep. There would need to be a cast from void * to struct mystruct *
    at the beginning of the callback as you propose here. I prefer this cast
    to be explicit (as proposed here) rather than hidden within the entire
    function call (void *) cast.

    >
    > >
    > > >
    > > > >
    > > > > I also don't trust that these complex TRACE_EVENT() preprocessor macros
    > > >
    > > > Thanks for your vote of confidence.
    > >
    > > Please don't take this personally. As I said above, I include myself in
    > > the list of people I don't trust to write entirely bug-free code. I'm
    > > just saying that we should not overlook a possibility to detect more
    > > bugs automatically when we have one, especially if this results in no
    > > object code change.
    >
    > The point being is that this is not about buggy code, but the fact that
    > the same data is being used in two places, you want to test to make sure
    > it is the same. I don't see how this helps.

    See my comment above about specifically casting the void *data parameter
    rather than relying on casting of the whole callback function pointer
    type to void *.

    >
    >
    >
    > >
    > > >
    > > > > will never ever have bugs. That's just doomed to happen one day or
    > > > > another. Again, call me paranoid if you like, but I think adding this
    > > > > type checking is justified.
    > > >
    > > > Where do you add the typecheck?? As I said before, if the TRACE_EVENT()
    > > > macros are broken, then so will the typecheck, and it will not catch the
    > > > errors.
    > > >
    > > > Sure the event macros can have bugs, but if it does then it will have
    > > > bugs for all. Because it is automated. If there is a bug, it wont be
    > > > because of a missed type being passed in, it would be because of one of
    > > > the extra macros we have that processes the same type incorrectly.
    > > >
    > > > >
    > > > > I am providing the type check implementation in a separate email. It
    > > > > will need to be extended to support the extra data parameter you plan to
    > > > > add.
    > > >
    > > > I saw the patch, but how does it help?
    > > >
    > > > I use "proto" to make the tracepoint and the callback, so I can add
    > > > somewhere this "check_trace_callback_type_##name(proto)", but if the
    > > > macros break somehow, that means proto changed between two references of
    > > > it, but what keeps proto from breaking at both callback creation and the
    > > > typecheck.
    > > >
    > > > Basically, you are saying that somehow the argument "proto" can change
    > > > between two uses of it. I don't really see that happening, and I'm not
    > > > paranoid enough to think that's an issue. Adding checks that don't
    > > > really check anything, honestly I find a waste, and just more confusion
    > > > in the macros.
    > >
    > > In the TRACE_EVENT() case, without the extra "void *data" argument,
    > > it is indeed checking that the "proto" of the callback you create is
    > > that same as the "proto" expected by the tracepoint call. However, given
    > > that you plan on adding other parameters besides "proto", then the added
    > > type-checking makes more and more sense.
    >
    > But you can not test it! That's my point.
    >
    > The first part of proto will be the same, and that's all we can test.
    > But the data parameter that the DECLARE_TRACE() is going to create will
    > be void *. Which means we can't test it. This is something that C lacks,
    > and we could test it in C++ if we did this with templates. The only way
    > to test it is at runtime with a magic number in the data field.
    >
    > This is the same as the file->private data. You can't test it at build
    > time.
    >
    > Let me explain this again:
    >
    > DECLARE_TRACE(name, proto, args);
    >
    > Will call the function like:
    >
    > callback(args, data);
    >
    > The callback will be at best:
    >
    > int callback(proto, void *data);
    >
    >
    > because the data being passed in is not defined yet. It is defined at
    > the point of the registering of the callback. You can have two callbacks
    > registered to the same tracepoint with two different types as the data
    > field.
    >
    > So what is it that this check is testing?

    It's making sure that TRACE_EVENT() creates callbacks with the following
    signature:

    void callback(proto, void *data)

    rather than

    void callback(proto, struct somestruct *data)

    and forces the cast to be done within the callback rather than casting
    the whole function pointer type to void *, assuming types to match. I
    prefer to leave the cast outside of the tracepoint infrastructure, so we
    do not obfuscate the fact that an explicit type cast is needed there.

    Thanks,

    Mathieu

    >
    > -- Steve
    >
    >

    --
    Mathieu Desnoyers
    Operating System Efficiency R&D Consultant
    EfficiOS Inc.
    http://www.efficios.com


    \
     
     \ /
      Last update: 2010-04-30 22:11    [W:0.031 / U:59.276 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site