diff mbox series

Fix: tracepoint.h: define SDT_USE_VARIADIC in pkg-config CFLAGS

Message ID 20200624160934.205881-1-stefanha@redhat.com
State New
Headers show
Series Fix: tracepoint.h: define SDT_USE_VARIADIC in pkg-config CFLAGS | expand

Commit Message

Stefan Hajnoczi June 24, 2020, 4:09 p.m. UTC
LTTng Userspace Tracer 2.11 with sdt enabled can break builds of
applications that include <sys/sdt.h> themselves. QEMU
(https://qemu.org/) is one example.

When applications include <sys/sdt.h> themselves before LTTng Userspace
Tracer header files and the SDT_USE_VARIADIC macro is not defined,
compilation fails due to the absense of STAP_PROBEV() macro that LTTng
Userspace Tracer requires.

Define SDT_USE_VARIADIC in the pkg-config file so that all compilation
units that use LTTng Userspace Tracer are guaranteed to have the macro
defined even if <sys/sdt.h> is included elsewhere before LTTng Userspace
Tracer header files.

In other words, define SDT_USE_VARIADIC on the compiler command-line
instead of in <lttng/tracepoint.h>. This way it will always been defined
when <sys/sdt.h> is included.

Make sure to define SDT_USE_VARIADIC when <lttng/tracepoint.h> is
included without it. This happens when applications do not use
pkg-config.

Fixes: b2f60c7986bb69f81b79b68f1bfe83aafa3278a7 ("Add sdt.h integration")
Signed-off-by: Stefan Hajnoczi <stefanha at redhat.com>
---
Warning: this patch doesn't work yet. SDT_CFLAGS also needs to be passed
into LTTng Userspace Tracer's own CFLAGS so that compilation can
succeed. I don't know the autoconf/automake syntax to do this and wanted
to see what you think of this solution before investing more time.
---
 include/lttng/tracepoint.h | 2 ++
 configure.ac               | 2 ++
 lttng-ust.pc.in            | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

Comments

Michael Jeanson June 26, 2020, 9:02 p.m. UTC | #1
Hi Stefan,

I proposed a patch here[1] that addresses this problem in a generic way by
duplicating the macro and namespacing it under LTTNG_. Honestly this is more
of an sdt bug, the STAP_PROBEV should be in it's own include guard outside
of _SYS_SDT_H or in it's own header file but we have to work with what's there.

Can you have a look at the patch and confirm that it fixes your problem?

Cheers,

Michael

[1] https://review.lttng.org/c/lttng-ust/+/3685 


----- On 24 Jun, 2020, at 12:09, lttng-dev lttng-dev at lists.lttng.org wrote:

> LTTng Userspace Tracer 2.11 with sdt enabled can break builds of
> applications that include <sys/sdt.h> themselves. QEMU
> (https://qemu.org/) is one example.
> 
> When applications include <sys/sdt.h> themselves before LTTng Userspace
> Tracer header files and the SDT_USE_VARIADIC macro is not defined,
> compilation fails due to the absense of STAP_PROBEV() macro that LTTng
> Userspace Tracer requires.
> 
> Define SDT_USE_VARIADIC in the pkg-config file so that all compilation
> units that use LTTng Userspace Tracer are guaranteed to have the macro
> defined even if <sys/sdt.h> is included elsewhere before LTTng Userspace
> Tracer header files.
> 
> In other words, define SDT_USE_VARIADIC on the compiler command-line
> instead of in <lttng/tracepoint.h>. This way it will always been defined
> when <sys/sdt.h> is included.
> 
> Make sure to define SDT_USE_VARIADIC when <lttng/tracepoint.h> is
> included without it. This happens when applications do not use
> pkg-config.
> 
> Fixes: b2f60c7986bb69f81b79b68f1bfe83aafa3278a7 ("Add sdt.h integration")
> Signed-off-by: Stefan Hajnoczi <stefanha at redhat.com>
> ---
> Warning: this patch doesn't work yet. SDT_CFLAGS also needs to be passed
> into LTTng Userspace Tracer's own CFLAGS so that compilation can
> succeed. I don't know the autoconf/automake syntax to do this and wanted
> to see what you think of this solution before investing more time.
Stefan Hajnoczi June 29, 2020, 4:26 p.m. UTC | #2
On Fri, Jun 26, 2020 at 05:02:00PM -0400, Michael Jeanson wrote:
> I proposed a patch here[1] that addresses this problem in a generic way by
> duplicating the macro and namespacing it under LTTNG_. Honestly this is more
> of an sdt bug, the STAP_PROBEV should be in it's own include guard outside
> of _SYS_SDT_H or in it's own header file but we have to work with what's there.
> 
> Can you have a look at the patch and confirm that it fixes your problem?

Great, glad there is already a fix under review.

When I try out the patch the application does not compile. The error
messages are the same as without the patch because _LTTNG_SDT_PROBE() is
not defined. Was that supposed to be _SDT_PROBE() instead?

The application compiles successfully after replacing _LTTNG_SDT_PROBE()
with _SDT_PROBE().

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <https://lists.lttng.org/pipermail/lttng-dev/attachments/20200629/7e417add/attachment.sig>
Michael Jeanson June 29, 2020, 4:36 p.m. UTC | #3
----- On 29 Jun, 2020, at 12:26, Stefan Hajnoczi stefanha at redhat.com wrote:

> On Fri, Jun 26, 2020 at 05:02:00PM -0400, Michael Jeanson wrote:
>> I proposed a patch here[1] that addresses this problem in a generic way by
>> duplicating the macro and namespacing it under LTTNG_. Honestly this is more
>> of an sdt bug, the STAP_PROBEV should be in it's own include guard outside
>> of _SYS_SDT_H or in it's own header file but we have to work with what's there.
>> 
>> Can you have a look at the patch and confirm that it fixes your problem?
> 
> Great, glad there is already a fix under review.
> 
> When I try out the patch the application does not compile. The error
> messages are the same as without the patch because _LTTNG_SDT_PROBE() is
> not defined. Was that supposed to be _SDT_PROBE() instead?
> 
> The application compiles successfully after replacing _LTTNG_SDT_PROBE()
> with _SDT_PROBE().
> 
> Stefan

Yes indeed, I reworked the patch after testing it and didn't catch this, I've
just pushed an updated patch. Thanks for testing it and catching this.

Cheers,

Michael
Stefan Hajnoczi June 30, 2020, 9:05 a.m. UTC | #4
On Mon, Jun 29, 2020 at 12:36:57PM -0400, Michael Jeanson wrote:
> ----- On 29 Jun, 2020, at 12:26, Stefan Hajnoczi stefanha at redhat.com wrote:
> 
> > On Fri, Jun 26, 2020 at 05:02:00PM -0400, Michael Jeanson wrote:
> >> I proposed a patch here[1] that addresses this problem in a generic way by
> >> duplicating the macro and namespacing it under LTTNG_. Honestly this is more
> >> of an sdt bug, the STAP_PROBEV should be in it's own include guard outside
> >> of _SYS_SDT_H or in it's own header file but we have to work with what's there.
> >> 
> >> Can you have a look at the patch and confirm that it fixes your problem?
> > 
> > Great, glad there is already a fix under review.
> > 
> > When I try out the patch the application does not compile. The error
> > messages are the same as without the patch because _LTTNG_SDT_PROBE() is
> > not defined. Was that supposed to be _SDT_PROBE() instead?
> > 
> > The application compiles successfully after replacing _LTTNG_SDT_PROBE()
> > with _SDT_PROBE().
> > 
> > Stefan
> 
> Yes indeed, I reworked the patch after testing it and didn't catch this, I've
> just pushed an updated patch. Thanks for testing it and catching this.

Nice! Thanks for the fix.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <https://lists.lttng.org/pipermail/lttng-dev/attachments/20200630/d2e3ebf9/attachment.sig>
diff mbox series

Patch

diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
index 7cfae3e4..6b699373 100644
--- a/include/lttng/tracepoint.h
+++ b/include/lttng/tracepoint.h
@@ -35,7 +35,9 @@ 
 #include <lttng/ust-compiler.h>
 
 #ifdef LTTNG_UST_HAVE_SDT_INTEGRATION
+#ifndef SDT_USE_VARIADIC
 #define SDT_USE_VARIADIC
+#endif
 #include <sys/sdt.h>
 #define LTTNG_STAP_PROBEV STAP_PROBEV
 #else
diff --git a/configure.ac b/configure.ac
index eb1bb3e6..bb3eeb60 100644
--- a/configure.ac
+++ b/configure.ac
@@ -427,6 +427,7 @@  AS_IF([test "x$with_sdt" = "xyes"], [
 	]])], [
 		AC_MSG_RESULT([yes])
 		AC_DEFINE([LTTNG_UST_HAVE_SDT_INTEGRATION], [1])
+		SDT_CFLAGS="-DSDT_USE_VARIADIC"
 	], [
 		AC_MSG_RESULT([no])
 		AC_MSG_ERROR([The sdt.h integration was requested but the STAP_PROBEV define cannot be used. Make sure it is installed, and up to date, or use CPPFLAGS=-I/path/ to specify a non-standard path to sys/sdt.h])
@@ -522,6 +523,7 @@  _AC_DEFINE_AND_SUBST([LTTNG_UST_DEFAULT_CONSTRUCTOR_TIMEOUT_MS], [3000])
 AM_CFLAGS="-Wall"
 AC_SUBST(AM_CFLAGS)
 AC_SUBST(JNI_CPPFLAGS)
+AC_SUBST(SDT_CFLAGS)
 
 AC_CONFIG_FILES([
 	Makefile
diff --git a/lttng-ust.pc.in b/lttng-ust.pc.in
index f9adde87..1b96db06 100644
--- a/lttng-ust.pc.in
+++ b/lttng-ust.pc.in
@@ -9,5 +9,5 @@  Version: @PACKAGE_VERSION@
 Requires:
 Requires.private: liburcu-bp
 Libs: -L${libdir} -llttng-ust -ldl
-Cflags: -I${includedir} 
+Cflags: -I${includedir} @SDT_CFLAGS@