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