[lttng-dev] [PATCH lttng-ust] Fix: remove weak attribute from __start___tracepoints_ptrs and __stop___tracepoints_ptrs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Gerlando Falauto" <gerlando.falauto at keymile.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers at efficios.com>
> Cc: lttng-dev at lists.lttng.org
> Sent: Tuesday, June 10, 2014 3:13:54 AM
> Subject: Re: [lttng-dev] [PATCH lttng-ust] Fix: remove weak attribute from __start___tracepoints_ptrs and
> __stop___tracepoints_ptrs
> 
> Hi Mathieu,
> 
> On 06/05/2014 07:27 PM, Mathieu Desnoyers wrote:
> > ----- Original Message -----
> >> From: "Gerlando Falauto" <gerlando.falauto at keymile.com>
> >> To: lttng-dev at lists.lttng.org
> >> Cc: "Gerlando Falauto" <gerlando.falauto at keymile.com>
> >> Sent: Wednesday, May 28, 2014 7:05:52 PM
> >> Subject: [lttng-dev] [PATCH lttng-ust] Fix: remove weak attribute from
> >> 	__start___tracepoints_ptrs and
> >> __stop___tracepoints_ptrs
> >>
> >> Some OpenEmbedded GCC releases (namely 4.7.2) incorrectly emit those
> >> symbols with default visibility if both weak and hidden attributes
> >> are used.
> >> When tracepoints are distributed among the main application and one or
> >> several shared objects (e.g. lttng_ust_tracef:event in liblttng-ust.so
> >> AND your own tracepoint provider, statically or dynamically linked),
> >> this results in a subtle name clash at runtime, causing only the
> >> tracepoints from one particular binary to be active, silently breaking
> >> all others.
> >> These symbols are indeed only declared and need not be defined (contrary
> >> to __tracepoint_registered and friends), as they are automatically
> >> PROVIDE-d by the linker as pointers to the "_tracepoint_ptrs" orphaned
> >> section.
> >
> > Does it work well if the program and/or libraries do the following:
> >
> > - they include tracepoint.h,
> > - but they don't contain any tracepoint (so there is technically no
> >    _tracepoint_ptrs section)
> >
> > Has this case been tested ? I remember that the weak was there for this
> > peculiar corner case.
> 
> I tested it exactly the way you described, and yes, it works.
> *HOWEVER*, if prior to including tracepoint.h the program also #defines
> TRACEPOINT_DEFINE, *THEN* we have the problem you described:
> 
> root at kmeter1:~# cat foo.c
> 
> #define TRACEPOINT_DEFINE
> #include <lttng/tracepoint.h>
> 
> int main(void)
> {
>    printf("Hello world!\n");
> }
> root at kmeter1:~# gcc foo.c -llttng-ust -ldl
> /tmp/cc92SYHX.o: In function `__tracepoints__ptrs_init':
> foo.c:(.text+0x39a): undefined reference to `__stop___tracepoints_ptrs'
> foo.c:(.text+0x39e): undefined reference to `__stop___tracepoints_ptrs'
> foo.c:(.text+0x3a2): undefined reference to `__start___tracepoints_ptrs'
> foo.c:(.text+0x3a6): undefined reference to `__start___tracepoints_ptrs'
> foo.c:(.text+0x3b2): undefined reference to `__start___tracepoints_ptrs'
> foo.c:(.text+0x3b6): undefined reference to `__start___tracepoints_ptrs'
> /tmp/cc92SYHX.o: In function `__tracepoints__ptrs_destroy':
> foo.c:(.text+0x446): undefined reference to `__start___tracepoints_ptrs'
> /tmp/cc92SYHX.o:foo.c:(.text+0x44a): more undefined references to
> `__start___tracepoints_ptrs' follow
> /usr/lib/gcc/powerpc-linux/4.7.2/../../../../powerpc-linux/bin/ld:
> a.out: hidden symbol `__stop___tracepoints_ptrs' isn't defined
> /usr/lib/gcc/powerpc-linux/4.7.2/../../../../powerpc-linux/bin/ld: final
> link failed: Bad value
> collect2: error: ld returned 1 exit status
> 
> Re-adding the weak attribute "fixes" this (though it leaves us with a
> much bigger issue, in my opinion).
> 
> However, I'm not sure whether:
> 
> a) this is really a use case (defining TRACEPOINT_DEFINE and then not
> really defining any), and even so, whether
> 
> b) what we have with the weak attribute is really what we want. In the
> end, we would be defining a function with __attribute__((constructor))
> (oh wait, potentially even more than one!) without actually having any
> tracepoint available. OK, the above complaints are probably not the most
> intuitive in the world, but isn't that better than an application
> silently "failing" (to provide any tracepoint) at runtime?
> 
> After all, the issue with the above messages is about the linker trying
> to resolve those symbols; but that's because they are used by
> __tracepoints__ptrs_init() and friends, not because they are merely
> declared.
> 
> Any thoughts?

I'm really not keen on changing the behavior of existing public APIs
to work around specific compiler bugs. Perhaps we could find a way to
tell users that they are facing this issue by adding a check in lttng-ust
configure.ac ?

Thanks,

Mathieu

> 
> Thank you!
> Gerlando
> 
> 
> >
> > Thanks,
> >
> > Mathieu
> >
> >>
> >> So even though the issue is only observed with a buggy compiler, removing
> >> the weak attribute also enforces the correct strong linking to the symbols
> >> provided by the linker (which would otherwise, by weak semantics, default
> >> to a value of all-zeroes).
> >>
> >> References: https://gcc.gnu.org/ml/gcc-help/2014-05/msg00005.html
> >> References:
> >> http://lists.lttng.org/pipermail/lttng-dev/2014-May/023090.html
> >> Reported-by: Martin Ünsal <martinunsal at gmail.com>
> >> Thanks-to: Henrik Wallin <henrikwallin72 at gmail.com>
> >> Cc: Paul Woegerer <Paul_Woegerer at mentor.com>
> >> Signed-off-by: Gerlando Falauto <gerlando.falauto at keymile.com>
> >> ---
> >>   include/lttng/tracepoint.h | 15 ++++++++++-----
> >>   1 file changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
> >> index c5a09d8..229ea72 100644
> >> --- a/include/lttng/tracepoint.h
> >> +++ b/include/lttng/tracepoint.h
> >> @@ -308,14 +308,19 @@ __tracepoints__destroy(void)
> >>   #ifdef TRACEPOINT_DEFINE
> >>
> >>   /*
> >> - * These weak symbols, the constructor, and destructor take care of
> >> - * registering only _one_ instance of the tracepoints per shared-ojbect
> >> - * (or for the whole main program).
> >> + * These strong hidden symbols are automatically provided by the linker
> >> + * for each shared-object (or for the whole main program) as pointers
> >> + * to the orphaned section "_tracepoints_ptrs" and must not be visible
> >> + * from other shared objects to avoid name clashes at runtime which would
> >> + * silently enable only the tracepoints from the object loaded first.
> >> + * NOTICE: Some OpenEmbedded GCC releases (namely 4.7.2) incorrectly
> >> + * emit those symbols with default visibility if both weak and hidden
> >> + * are used.
> >>    */
> >>   extern struct tracepoint * const __start___tracepoints_ptrs[]
> >> -	__attribute__((weak, visibility("hidden")));
> >> +	__attribute__((visibility("hidden")));
> >>   extern struct tracepoint * const __stop___tracepoints_ptrs[]
> >> -	__attribute__((weak, visibility("hidden")));
> >> +	__attribute__((visibility("hidden")));
> >>
> >>   /*
> >>    * When TRACEPOINT_PROBE_DYNAMIC_LINKAGE is defined, we do not emit a
> >> --
> >> 1.8.0.1
> >>
> >>
> >> _______________________________________________
> >> lttng-dev mailing list
> >> lttng-dev at lists.lttng.org
> >> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >>
> >
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com