----- Original Message ----- > From: "Gerlando Falauto" <[email protected]> > To: "Mathieu Desnoyers" <[email protected]> > Cc: [email protected] > 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 <[email protected]> > >>>> Thanks-to: Henrik Wallin <[email protected]> > >>>> Cc: Paul Woegerer <[email protected]> > >>>> Signed-off-by: Gerlando Falauto <[email protected]> > >>>> --- > >>>> 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 > >>>> [email protected] > >>>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > >>>> > >>> > >> > >> > > > > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
