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

[Gerlando Falauto]

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?

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