lkml.org 
[lkml]   [2010]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 04/10][RFC] tracing: Remove per event trace registering
    From
    Date
    On Fri, 2010-04-30 at 13:09 -0400, Mathieu Desnoyers wrote:

    > How can you be sure that the "void *data" type will match the type at
    > the same position in the generated callback ?


    We do it all the time in the kernel with no type checking. Just look at
    all the users of file->private.


    >
    > Honestly, I don't think kernel programmers write bug-free code. And I
    > include myself when I say that. So the best we can do, on top of code
    > review, is to use all the verification and debugging tools available to
    > minimize the amount of undetected bugs. Rather than try to find out the
    > cause of subtly broken tracepoint callbacks with their runtime
    > side-effects, I strongly prefer to let the compiler find this out as
    > early as possible.

    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.

    >
    > I also don't trust that these complex TRACE_EVENT() preprocessor macros

    Thanks for your vote of confidence.

    > 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.

    -- Steve


    > >
    > >
    > > > >
    > > > > The callback is created in include/trace/ftrace.h:
    > > > >
    > > > > #undef TRACE_EVENT
    > > > > #define TRACE_EVENT(name, proto, args, tstuct, assign, print) \
    > > > > DECLARE_EVENT_CLASS(name, \
    > > > > PARAMS(proto), \
    > > > > PARAMS(args), \
    > > > > PARAMS(tstruct), \
    > > > > PARAMS(assign), \
    > > > > PARAMS(print)); \
    > > > > DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args));
    > > > >
    > > > > #undef DECLARE_EVENT_CLASS
    > > > > #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
    > > > > \
    > > > > static notrace void \
    > > > > ftrace_raw_event_##call(proto, \
    > > > > struct ftrace_event_call *event_call) \
    > > > > [...]
    > > > >
    > > >
    > > > Either within this callback, or in a dummy static function after, we
    > > > could add:
    > > >
    > > > check_trace_##call(ftrace_raw_event_##call);
    > > >
    > > > So.. you are the preprocessor expert, do you think this could fly ? ;)
    > >
    > >
    > >
    > > Sure, the static function you did could be added, and hope that gcc is
    > > smart enough to get rid of it (add __unused to it). But what are we
    > > really checking here? If CPP works?
    > >
    > > -- Steve
    > >
    > >
    >




    \
     
     \ /
      Last update: 2010-04-30 20:21    [W:0.032 / U:0.280 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site