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

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

[Mathieu Desnoyers]

----- Original Message -----
> From: "Gerlando Falauto" <gerlando.falauto@keymile.com>
> To: lttng-dev@lists.lttng.org
> Cc: "Gerlando Falauto" <gerlando.falauto@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.

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@gmail.com>
> Thanks-to: Henrik Wallin <henrikwallin72@gmail.com>
> Cc: Paul Woegerer <Paul_Woegerer@mentor.com>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@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@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

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

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [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@keymile.com>
>> To: lttng-dev@lists.lttng.org
>> Cc: "Gerlando Falauto" <gerlando.falauto@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@kmeter1:~# cat foo.c

#define TRACEPOINT_DEFINE
#include <lttng/tracepoint.h>

int main(void)
{
   printf("Hello world!\n");
}
root@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@gmail.com>
>> Thanks-to: Henrik Wallin <henrikwallin72@gmail.com>
>> Cc: Paul Woegerer <Paul_Woegerer@mentor.com>
>> Signed-off-by: Gerlando Falauto <gerlando.falauto@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@lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>
>


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers]

----- Original Message -----
> From: "Gerlando Falauto" <gerlando.falauto@keymile.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: lttng-dev@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@keymile.com>
> >> To: lttng-dev@lists.lttng.org
> >> Cc: "Gerlando Falauto" <gerlando.falauto@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@kmeter1:~# cat foo.c
> 
> #define TRACEPOINT_DEFINE
> #include <lttng/tracepoint.h>
> 
> int main(void)
> {
>    printf("Hello world!\n");
> }
> root@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@gmail.com>
> >> Thanks-to: Henrik Wallin <henrikwallin72@gmail.com>
> >> Cc: Paul Woegerer <Paul_Woegerer@mentor.com>
> >> Signed-off-by: Gerlando Falauto <gerlando.falauto@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@lists.lttng.org
> >> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >>
> >
> 
> 

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

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Gerlando Falauto]

Hi Mathieu,

[... removing weak from __attribute__(...) of __start___tracepoints_ptrs 
and __stop___tracepoints_ptrs]

>> Any thoughts?
>
> I'm really not keen on changing the behavior of existing public APIs
> to work around specific compiler bugs.

I'm not keen on that, either. But how can you be so sure this is really 
the case? I'm not a compiler expert, and I don't understand the inner 
workings of lttng-ust either, but I really don't see a reason why those 
symbols should be both weak *and* hidden.
As far as I could understand, those symbols *have* to be defined in 
order for the various contructors to work properly. It's just *the way* 
they are defined that's a bit weak (or weird), in that the linker's the 
one defining them instead of the user (through the compiler).
If they're not defined (i.e. because no tracepoint is defined at all), 
no one should use them either (i.e. no 
__tracepoints__ptrs_init()/__tracepoints__ptrs_destroy() function should 
be emitted, either). If someone inadvertently changes the section nome 
to something else, the linker should complain.
To me, just having the linker shut up and swallow it, seems plain wrong.

 > Perhaps we could find a way to
> tell users that they are facing this issue by adding a check in lttng-ust
> configure.ac ?

I'd thought of that, too. Maybe a #define symbol, generated by the 
configure script to distinguish between the two cases?
I believe the test case to detect this bug was quite trivial; I would 
just have to dig it up. But I'm not very fond of autotools, either. ;-)
Plus, I still don't see the issue with removing weak, really.

But I'm open to any suggestion.

Thanks again!
Gerlando

>
> 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@gmail.com>
>>>> Thanks-to: Henrik Wallin <henrikwallin72@gmail.com>
>>>> Cc: Paul Woegerer <Paul_Woegerer@mentor.com>
>>>> Signed-off-by: Gerlando Falauto <gerlando.falauto@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@lists.lttng.org
>>>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>>>
>>>
>>
>>
>


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers]

----- Original Message -----
> From: "Gerlando Falauto" <gerlando.falauto@keymile.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: lttng-dev@lists.lttng.org
> Sent: Tuesday, June 10, 2014 4:06:41 PM
> Subject: Re: [lttng-dev] [PATCH lttng-ust] Fix: remove weak attribute from __start___tracepoints_ptrs and
> __stop___tracepoints_ptrs
> 
> Hi Mathieu,
> 
> [... removing weak from __attribute__(...) of __start___tracepoints_ptrs
> and __stop___tracepoints_ptrs]
> 
> >> Any thoughts?
> >
> > I'm really not keen on changing the behavior of existing public APIs
> > to work around specific compiler bugs.
> 
> I'm not keen on that, either. But how can you be so sure this is really
> the case? I'm not a compiler expert, and I don't understand the inner
> workings of lttng-ust either, but I really don't see a reason why those
> symbols should be both weak *and* hidden.
> As far as I could understand, those symbols *have* to be defined in
> order for the various contructors to work properly. It's just *the way*
> they are defined that's a bit weak (or weird), in that the linker's the
> one defining them instead of the user (through the compiler).
> If they're not defined (i.e. because no tracepoint is defined at all),
> no one should use them either (i.e. no
> __tracepoints__ptrs_init()/__tracepoints__ptrs_destroy() function should
> be emitted, either). If someone inadvertently changes the section nome
> to something else, the linker should complain.
> To me, just having the linker shut up and swallow it, seems plain wrong.

The tracepoints being defined are marshalled by the constructor. If we think
of a case where a user is defining the TRACEPOINT_DEFINE before including
an instrumentation header, and this instrumentation header happens to be
empty (e.g. because some preprocessor ifdefs are disabling all of its
tracepoints), we don't want to trigger a linking issue due to unavailability
of start/stop tracepoints_ptrs symbols. Hence the "weak".

The "hidden" attribute is important to ensure that the symbol stays local
to each .so/executable.

> 
>  > Perhaps we could find a way to
> > tell users that they are facing this issue by adding a check in lttng-ust
> > configure.ac ?
> 
> I'd thought of that, too. Maybe a #define symbol, generated by the
> configure script to distinguish between the two cases?
> I believe the test case to detect this bug was quite trivial; I would
> just have to dig it up. But I'm not very fond of autotools, either. ;-)
> Plus, I still don't see the issue with removing weak, really.
> 
> But I'm open to any suggestion.

So we're talking about a bug that is specific to a compiler of a given
distribution, due to a port of a patch specific to this distribution,
am I correct ? If it is indeed the case, can we detect that we are using
the compiler from this distribution, perhaps with a specific "define"
that is put there by the distribution vendor ?

Having the output of gcc -dM -E - < /dev/null   and   gcc --version on
both the problematic compiler and an "upstream" vanilla compiler of the
same version could perhaps help us find a way to identify the
distro-specific compiler ?

Thanks,

Mathieu

> 
> Thanks again!
> Gerlando
> 
> >
> > 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@gmail.com>
> >>>> Thanks-to: Henrik Wallin <henrikwallin72@gmail.com>
> >>>> Cc: Paul Woegerer <Paul_Woegerer@mentor.com>
> >>>> Signed-off-by: Gerlando Falauto <gerlando.falauto@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@lists.lttng.org
> >>>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> >>>>
> >>>
> >>
> >>
> >
> 
> 

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

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Thibault, Daniel]

----------------------------------------------------------------------
Date: Tue, 10 Jun 2014 09:13:54 +0200

>> 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@kmeter1:~# cat foo.c
>
>#define TRACEPOINT_DEFINE
>#include <lttng/tracepoint.h>
>
>int main(void)
>{
>   printf("Hello world!\n");
>}
>root@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'
>[...]
>/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?
>
>Gerlando
----------------------------------------------------------------------

   If the issue is to protect against TRACEPOINT_DEFINE not being 'fulfilled' before hitting lttng/tracepoint.h, maybe tracepoint.h should check for TRACEPOINT_PROVIDER in addition to TRACEPOINT_DEFINE.

Daniel U. Thibault
Protection des systèmes et contremesures (PSC) | Systems Protection & Countermeasures (SPC)
Cyber sécurité pour les missions essentielles (CME) | Mission Critical Cyber Security (MCCS)
RDDC - Centre de recherches de Valcartier | DRDC - Valcartier Research Centre
2459 route de la Bravoure
Québec QC  G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC : 918V QSDJ <http://www.travelgis.com/map.asp?addr=918V%20QSDJ>
Gouvernement du Canada | Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

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

[Gerlando Falauto]

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.

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@gmail.com>
Thanks-to: Henrik Wallin <henrikwallin72@gmail.com>
Cc: Paul Woegerer <Paul_Woegerer@mentor.com>
Signed-off-by: Gerlando Falauto <gerlando.falauto@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@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev