[lttng-dev] [PATCH] Define tracepoint callback only if TRACEPOINT_DEFINE defined

[Zifei Tong]

On Thu, Mar 13, 2014 at 9:49 PM, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> ----- Original Message -----
>> From: "Zifei Tong" <zifeitong at gmail.com>
>> To: "mathieu desnoyers" <mathieu.desnoyers at efficios.com>
>> Cc: lttng-dev at lists.lttng.org, "Zifei Tong" <zifeitong at gmail.com>
>> Sent: Wednesday, March 12, 2014 5:40:37 AM
>> Subject: [PATCH] Define tracepoint callback only if TRACEPOINT_DEFINE defined
>>
>> Compiling tracepoint providers in clang will result in 'Wunused-function'
>> warning, since tracepoint callbacks are defined but never called.
>>
>> lttng-gen-tp is also updated to not put TRACEPOINT_DEFINE in generated
>> headers.
>>
>> Fixes #760
>>
>> Signed-off-by: Zifei Tong <zifeitong at gmail.com>
>> ---
>>  doc/examples/gen-tp/sample.c |  1 +
>>  include/lttng/tracepoint.h   | 13 ++++++++++---
>>  tools/lttng-gen-tp           |  1 -
>>  3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
>> index 66e2abd..462a0fc 100644
>> --- a/include/lttng/tracepoint.h
>> +++ b/include/lttng/tracepoint.h
>> @@ -150,8 +150,8 @@ extern "C" {
>>   * between caller's ip addresses within the probe using the return
>>   * address.
>>   */
>> -#define _DECLARE_TRACEPOINT(_provider, _name, ...)                                   \
>> -extern struct tracepoint __tracepoint_##_provider##___##_name;                               \
>> +#if defined(TRACEPOINT_DEFINE)
>
> So you are adding a ifdef conditional to skip creation of a macro that defines
> static inline functions. What is it achieving really other than silencing a compiler
> warning ? Anyway the compiler will optimize those static inline away if they are
> not used.

I just want to get rid of these warnings (I am little bit paranoid for
compiler warnings especially for clang's colorful ones :D). I think
these warnings result from compiling a simple 'hello-world' tracepoint
provider are annoying and confusing.

> I think this is a case where we might want to use __attribute__((unused)) to just
> shut up the clang warning.

Yes, with __attribute__((unused)), a one-line patch would be enough to
silence these warnings.
And I've tested with clang, __attribute__((unused)) works well.

>> diff --git a/doc/examples/gen-tp/sample.c b/doc/examples/gen-tp/sample.c
>> index dab6e7d..ae22f28 100644
>> --- a/doc/examples/gen-tp/sample.c
>> +++ b/doc/examples/gen-tp/sample.c
>> @@ -23,6 +23,7 @@
>>
>>  #include <unistd.h>
>>
>> +#define TRACEPOINT_DEFINE
>
> Hrm, so this requires end users to change the way to use lttng-gen-tp.
> I'm not sure I like that at all. This means changes in the way people do
> instrumentation, and I'm trying really hard not to break that.
>
>>  #include "sample_tracepoint.h"
>>  int main(int argc, char **argv)
>>  {

There is no need to change lttng-gen-tp anymore.

I'll send a new simple patch for your review.

Thank you!
Zifei Tong

> Thoughts ?
>
> Thanks,
>
> Mathieu
>
>> +#define _DECLARE_TRACEPOINT_CB(_provider, _name, ...)                                        \
>>  static inline __attribute__((always_inline)) lttng_ust_notrace                               \
>>  void __tracepoint_cb_##_provider##___##_name(_TP_ARGS_PROTO(__VA_ARGS__));
>>               \
>>  static                                                                                       \
>> @@ -174,7 +174,14 @@ void
>> __tracepoint_cb_##_provider##___##_name(_TP_ARGS_PROTO(__VA_ARGS__))          \
>>       } while ((++__tp_probe)->func);                                                 \
>>  end:                                                                                 \
>>       tp_rcu_read_unlock_bp();                                                        \
>> -}                                                                                    \
>> +}
>> +#else
>> +#define _DECLARE_TRACEPOINT_CB(_provider, _name, ...)
>> +#endif
>> +
>> +#define _DECLARE_TRACEPOINT(_provider, _name, ...)                                   \
>> +extern struct tracepoint __tracepoint_##_provider##___##_name;                               \
>> +_DECLARE_TRACEPOINT_CB(_provider, _name, __VA_ARGS__)                                        \
>>  static inline lttng_ust_notrace                                                              \
>>  void __tracepoint_register_##_provider##___##_name(char *name,                               \
>>               void (*func)(void), void *data);                                        \
>> diff --git a/tools/lttng-gen-tp b/tools/lttng-gen-tp
>> index c49e8a5..ff8c22d 100755
>> --- a/tools/lttng-gen-tp
>> +++ b/tools/lttng-gen-tp
>> @@ -69,7 +69,6 @@ class CFile:
>>  /*
>>   * The header containing our TRACEPOINT_EVENTs.
>>   */
>> -#define TRACEPOINT_DEFINE
>>  #include "{headerFilename}"
>>  """
>>      def __init__(self, filename, template):
>> --
>> 1.9.0
>>
>>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com