From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Maxime_Roussin=2DB=C3=A9langer?= Subject: Re: [PATCH] Introduce vtracef Date: Fri, 31 Jan 2020 18:17:24 -0500 Message-ID: References: <20200131215501.19536-1-maxime.roussinbelanger@gmail.com> <569392e8-be60-26e1-a2d4-72112e6acbac@simark.ca> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3542073465027032504==" Return-path: Received: from mail-io1-xd43.google.com (mail-io1-xd43.google.com [IPv6:2607:f8b0:4864:20::d43]) by lists.lttng.org (Postfix) with ESMTPS id 488Y6R66r6z24PP for ; Fri, 31 Jan 2020 18:17:51 -0500 (EST) Received: by mail-io1-xd43.google.com with SMTP id z16so5403768iod.11 for ; Fri, 31 Jan 2020 15:17:51 -0800 (PST) In-Reply-To: <569392e8-be60-26e1-a2d4-72112e6acbac@simark.ca> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: lttng-dev-bounces@lists.lttng.org Sender: "lttng-dev" To: Simon Marchi Cc: lttng-dev@lists.lttng.org, champagne.guillaume.c@gmail.com List-Id: lttng-dev@lists.lttng.org --===============3542073465027032504== Content-Type: multipart/alternative; boundary="0000000000004dff8b059d77c8a3" --0000000000004dff8b059d77c8a3 Content-Type: text/plain; charset="UTF-8" Hi Simon, On Fri, Jan 31, 2020 at 6:03 PM Simon Marchi wrote: > > Hi Maxime, > > Tip for next time: this mailing is used for many projects, so it helps > to include the project name in the patch prefix. You can do so by using > > --subject-prefix="PATCH lttng-ust" > > on you git-send-email command line. Will do! > > On 2020-01-31 4:55 p.m., Maxime Roussin-Belanger wrote: > > vtracef accepts a va_list argument to simplify tracing > > functions which use a va_list > > > > Here's an example from wpa_supplicant that I wanted to > > trace: > > > > void wpa_debug(int level, const char* fmt, ...) { > > > > va_list ap; > > va_start(ap, fmt); > > > > ... > > // The call I want to easily trace with vtracef > > vprintf(fmt, ap); > > > > ... > > va_end(ap); > > } > > > > wpa_debug is used a fair amount and it would be annoying to > > replace all the wpa_debug calls with tracef. > > > > With vtracef, it simplifies the find and replace effort by > > only changing it at one place. > > The shortcoming, as you mentiond on IRC, is that it doesn't support SDT integration. The tracef > macro call has a `STAP_PROBEV` call, but it doesn't seem possible to use that here. The number > of arguments in a va_list is known at runtime (in fact it's not really known by the va_list itself), > whereas STAP_PROBEV wants to know the number of arguments at compile time. > > I think it would still be useful to have vtracef, for the use case explained above, but have it > documented that it is not integrated with the stap probes (unless somebody has a magic idea that > would make it work). But that will be for the maintainer to decide. Where would that be? In the man page? If so it would be the first time editing/creating a man page. It seems fairly simple, but I probably need some guidance on where to start. > > Note that if the feature is accepted, it will require at least a man page update. An update in > doc/examples/demo-tracef/demo-tracef.c would also be nice, to have an example of how it can be used. Looking at the example, maybe I should create another file demo-vtracef.c? Max. > > Simon > > > > > Signed-off-by: Maxime Roussin-Belanger > > --- > > include/lttng/tracef.h | 10 ++++++++++ > > liblttng-ust/tracef.c | 18 +++++++++++------- > > 2 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/include/lttng/tracef.h b/include/lttng/tracef.h > > index 0c59c9ae..e3a7587d 100644 > > --- a/include/lttng/tracef.h > > +++ b/include/lttng/tracef.h > > @@ -32,13 +32,23 @@ extern "C" { > > extern > > void _lttng_ust_tracef(const char *fmt, ...); > > > > +extern > > +void _lttng_ust_vtracef(const char *fmt, va_list ap); > > + > > #define tracef(fmt, ...) \ > > do { \ > > LTTNG_STAP_PROBEV(tracepoint_lttng_ust_tracef, event, ## __VA_ARGS__); \ > > if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \ > > _lttng_ust_tracef(fmt, ## __VA_ARGS__); \ > > } while (0) > > > > +#ifndef LTTNG_UST_HAVE_SDT_INTEGRATION > > +#define vtracef(fmt, ap) \ > > + do { \ > > + if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.state)) \ > > + _lttng_ust_vtracef(fmt, ap); \ > > + } while (0) > > +#endif > > If we end up with what I suggested above (vtracef documented not to integrate an > stap probe), this #ifndef would go away, > > > #ifdef __cplusplus > > } > > #endif > > diff --git a/liblttng-ust/tracef.c b/liblttng-ust/tracef.c > > index ea98e43e..2a809eed 100644 > > --- a/liblttng-ust/tracef.c > > +++ b/liblttng-ust/tracef.c > > @@ -29,20 +29,24 @@ > > #define TRACEPOINT_DEFINE > > #include "lttng-ust-tracef-provider.h" > > > > -void _lttng_ust_tracef(const char *fmt, ...) > > +void _lttng_ust_vtracef(const char *fmt, va_list ap) > > { > > - va_list ap; > > char *msg; > > - int len; > > - > > - va_start(ap, fmt); > > - len = vasprintf(&msg, fmt, ap); > > + const int len = vasprintf(&msg, fmt, ap); > > /* len does not include the final \0 */ > > if (len < 0) > > - goto end; > > + return; > > We try to have a single exit point in each function, to facilitate > resource management in the case of errors, so please keep the goto > end pattern: > > ... > > if (len < 0) > goto end; > > ... > > end: > return > } > > > __tracepoint_cb_lttng_ust_tracef___event(msg, len, > > LTTNG_UST_CALLER_IP()); > > free(msg); > > +} > > + > > +void _lttng_ust_tracef(const char *fmt, ...) > > +{ > > + va_list ap; > > + > > + va_start(ap, fmt); > > + _lttng_ust_vtracef(fmt, ap); > > end: > > You can remove the end label here. > > Simon --0000000000004dff8b059d77c8a3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Simon,

On Fri, Jan 31, 2020 at 6:03 PM Simon Mar= chi <simark@simark.ca> wrote:=
>
> Hi Maxime,
>
> Tip for next time: this mailing= is used for many projects, so it helps
> to include the project name= in the patch prefix.=C2=A0 You can do so by using
>
> =C2=A0 -= -subject-prefix=3D"PATCH lttng-ust"
>
> on you git-se= nd-email command line.

Will do!

>
&g= t; On 2020-01-31 4:55 p.m., Maxime Roussin-Belanger wrote:
> > vtr= acef accepts a va_list argument to simplify tracing
> > functions = which use a va_list
> >
> > Here's an example from wp= a_supplicant that I wanted to
> > trace:
> >
> >= void wpa_debug(int level, const char* fmt, ...) {
> >
> >= ; =C2=A0 =C2=A0 va_list ap;
> > =C2=A0 =C2=A0 va_start(ap, fmt);> >
> > =C2=A0 =C2=A0 ...
> > =C2=A0 =C2=A0 // Th= e call I want to easily trace with vtracef
> > =C2=A0 =C2=A0 vprin= tf(fmt, ap);
> >
> > =C2=A0 =C2=A0 ...
> > =C2= =A0 =C2=A0 va_end(ap);
> > }
> >
> > wpa_debug i= s used a fair amount and it would be annoying to
> > replace all t= he wpa_debug calls with tracef.
> >
> > With vtracef, it = simplifies the find and replace effort by
> > only changing it at = one place.
>
> The shortcoming, as you mentiond on IRC, is that= it doesn't support SDT integration.=C2=A0 The tracef
> macro cal= l has a `STAP_PROBEV` call, but it doesn't seem possible to use that he= re.=C2=A0 The number
> of arguments in a va_list is known at runtime = (in fact it's not really known by the va_list itself),
> whereas = STAP_PROBEV wants to know the number of arguments at compile time.
><= br>> I think it would still be useful to have vtracef, for the use case = explained above, but have it
> documented that it is not integrated w= ith the stap probes (unless somebody has a magic idea that
> would ma= ke it work).=C2=A0 But that will be for the maintainer to decide.

Where would that be? In the man page? If so it would be the= first time editing/creating a man page.
It seems fairly simple, = but I probably need some guidance on where to start.

>
= > Note that if the feature is accepted, it will require at least a man p= age update. An update in
> doc/examples/demo-tracef/demo-tracef.c wou= ld also be nice, to have an example of how it can be used.

Looking at the example, maybe I should create another file demo-vt= racef.c?

Max.

>
> Simon
= >
> >
> > Signed-off-by: Maxime Roussin-Belanger <<= a href=3D"mailto:maxime.roussinbelanger@gmail.com">maxime.roussinbelanger@g= mail.com>
> > ---
> > =C2=A0include/lttng/tracef.h= | 10 ++++++++++
> > =C2=A0liblttng-ust/tracef.c =C2=A0| 18 ++++++= +++++-------
> > =C2=A02 files changed, 21 insertions(+), 7 deleti= ons(-)
> >
> > diff --git a/include/lttng/tracef.h b/incl= ude/lttng/tracef.h
> > index 0c59c9ae..e3a7587d 100644
> >= ; --- a/include/lttng/tracef.h
> > +++ b/include/lttng/tracef.h> > @@ -32,13 +32,23 @@ extern "C" {
> > =C2=A0ex= tern
> > =C2=A0void _lttng_ust_tracef(const char *fmt, ...);
&g= t; >
> > +extern
> > +void _lttng_ust_vtracef(const c= har *fmt, va_list ap);
> > +
> > =C2=A0#define tracef(fmt= , ...) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 \
> > =C2=A0 =C2=A0 =C2=A0 do { =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0\
> > =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LTTNG_STAP_PROBEV(tracepoint_lttng_ust_trac= ef, event, ## __VA_ARGS__); \
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 if (caa_unlikely(__tracepoint_lttng_ust_tracef___event.st= ate)) \
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 _lttng_ust_tracef(fmt, ## __VA_ARGS__); =C2=A0 =C2= =A0 =C2=A0 =C2=A0 \
> > =C2=A0 =C2=A0 =C2=A0 } while (0)
> &= gt;
> > +#ifndef LTTNG_UST_HAVE_SDT_INTEGRATION
> > +#de= fine vtracef(fmt, ap) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> > + =C2=A0 =C2=A0 do { =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0\
> > + =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (caa_unlikely(__tracepoint_lttng_ust= _tracef___event.state)) \
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 _lttng_ust_vtracef(fmt, ap); =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0\
> > + =C2=A0 =C2=A0 } while (0)> > +#endif
>
> If we end up with what I suggested abov= e (vtracef documented not to integrate an
> stap probe), this #ifndef= would go away,
>
> > =C2=A0#ifdef __cplusplus
> > = =C2=A0}
> > =C2=A0#endif
> > diff --git a/liblttng-ust/tr= acef.c b/liblttng-ust/tracef.c
> > index ea98e43e..2a809eed 100644=
> > --- a/liblttng-ust/tracef.c
> > +++ b/liblttng-ust/t= racef.c
> > @@ -29,20 +29,24 @@
> > =C2=A0#define TRACEPO= INT_DEFINE
> > =C2=A0#include "lttng-ust-tracef-provider.h&qu= ot;
> >
> > -void _lttng_ust_tracef(const char *fmt, ...= )
> > +void _lttng_ust_vtracef(const char *fmt, va_list ap)
>= ; > =C2=A0{
> > - =C2=A0 =C2=A0 va_list ap;
> > =C2=A0= =C2=A0 =C2=A0 char *msg;
> > - =C2=A0 =C2=A0 int len;
> >= ; -
> > - =C2=A0 =C2=A0 va_start(ap, fmt);
> > - =C2=A0 = =C2=A0 len =3D vasprintf(&msg, fmt, ap);
> > + =C2=A0 =C2=A0 c= onst int len =3D vasprintf(&msg, fmt, ap);
> > =C2=A0 =C2=A0 = =C2=A0 /* len does not include the final \0 */
> > =C2=A0 =C2=A0 = =C2=A0 if (len < 0)
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 goto end;
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 r= eturn;
>
> We try to have a single exit point in each function,= to facilitate
> resource management in the case of errors, so please= keep the goto
> end pattern:
>
> =C2=A0 ...
>
&= gt; =C2=A0 if (len < 0)
> =C2=A0 =C2=A0 goto end;
>
> = =C2=A0 ...
>
> end:
> =C2=A0 return
> }
>
= > > =C2=A0 =C2=A0 =C2=A0 __tracepoint_cb_lttng_ust_tracef___event(msg= , len,
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LTTNG_= UST_CALLER_IP());
> > =C2=A0 =C2=A0 =C2=A0 free(msg);
> >= +}
> > +
> > +void _lttng_ust_tracef(const char *fmt, ..= .)
> > +{
> > + =C2=A0 =C2=A0 va_list ap;
> > +<= br>> > + =C2=A0 =C2=A0 va_start(ap, fmt);
> > + =C2=A0 =C2= =A0 _lttng_ust_vtracef(fmt, ap);
> > =C2=A0end:
>
> Yo= u can remove the end label here.
>
> Simon
--0000000000004dff8b059d77c8a3-- --===============3542073465027032504== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev --===============3542073465027032504==--