From a76c1d7c3466746c96dffd6406017d438bb22018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Fri, 6 Oct 2017 17:17:26 +0200 Subject: [PATCH 5/6] High: bare fix for libqb logging not working with ld.bfd/binutils 2.29+ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Initially with the help of the internal test suite and the failing log test, it was eventually discovered[1] that these binutils commits going to the recent 2.29 release affected the treatment of _start_SECNAME and __stop_SECNAME symbols denoting the boundary start/stop addresses of a SECNAME orphan section -- specifically in libqb context a custom section (SECNAME=__verbose) used for link-time ("run-time amortizing") callsite collection when there's a support in the toolchain[*]: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cbd0eecf261c2447781f8c89b0d955ee66fae7e9 https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b27685f2016c510d03ac9a64f7b04ce8efcf95c4 https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7dba9362c172f1073487536eb137feb2da30b0ff The first one explicitly states: > Also __start_SECNAME and __stop_SECNAME symbols are marked as hidden > by ELF linker so that __start_SECNAME and __stop_SECNAME symbols for > section SECNAME in different modules are unique. The problem is that libqb silently depends on the previous status quo ld.bfd linker behaviour of keeping those symbols externally visible, which was apparently not granted as it has deliberately changed per above. And then for 2.29.1 release of binutils once again, as someone actually noticed something went overboard with the 2.29 changes: https://sourceware.org/bugzilla/show_bug.cgi?id=21964 https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=487b6440dad57440939fab7afdd84a218b612796 At least that change doesn't invalidate all the effort being put into the original version of the changeset, only the configure script check had to be refined so as not to miss the "orphan section magic not working properly out of the box, without a bandaid" obseration (see the inline comment) -- the workaround arrangement needs to be applied in that case as well. * * * So regarding the solution itself, the core of the fix was sketched at the original Fedora targeted bug against binutils[2]. In short, we are using a custom linker script that (re)describes the mentioned custom orphan output section, or better yet, assuredly pushes that section, and more importantly, it's own boundary denoting symbols, through into the resulting binary when it's being linked (as in compile-time step). This solution alone, while working for the non-libqb (more on that below) logging participants, is not good enough, as it requires all libqb targets to start using new incantation (namely "-Wl,foo.t" switch) in the final link step during compilation, which might be solvable with a tweak in libqb's pkg-config file under assumption that practice of using "pkg-config --libs libqb" is rigidly followed. Which is likely a false expectation, and furthermore only for the regular consumption model, as it doesn't cover the least bit the developmental one (refer to previous-but-one "tests" commit message), e.g. applied for internal examples + tests (but no local sub-checkout tree usage can be excluded). So further extensions were devised to cover both consumption models: - a. regular: courtesy of binutils maintainer[3], we follow an idea to make libqb.so (i.e. what the targets link against) rather a linker script on its own, which first include the version-specified (e.g. libqb.so.0) file into the link, then lists, in situ, the content of the linker script per above, hence -lqb linking has the same effect as having both "-lqb -Wl,foo.t" explicitly in the link command prior to this trick - b. developmental: to eliminate any kind of race condition arising from the attempt to post-modify libqb.la libtool archive file generated internally by libtool, we sort of abuse "inherited_linker_flags" variable within this file format, as it forms an accumulative value across the whole transitive dependencies chain, which serves exactly our purpose of injecting "-Wl,foo.t" switch equivalent for those libtool-linking by L{D,IB}ADDing libqb.la; it's then enough to craft a custom libtool archive file declaring that value, and hook it into such dependency chain through libqb_la_LIBADD, and with a little bit of further fiddling, it works as desired (note that double occurrence of "-Wl,foo.t" equivalent achieved at some point when developing this trick turned to be, surprisingly, counter-productive) Last but not least, libqb itself needs to be linked with the mentioned "-Wl,foo.t" equivalent for its own outgoing log messages to be honoured, which is already achieved with the arrangement for b. above, and by experiments, further redefinition of those boundary denoting symbols as weak was necessary so as to make them truly global within libqb.so proper (at least with binutils 2.29). * * * Finally, let's have a look how the already well-known test matrix overview changes as of this commit, but first as a recap, "X(Y)" denotes "X linked with linker Y": X(a) .. ld.bfd < 2.29 X(b) .. ld.bfd = 2.29 (and likely on) and here you are (values in denote non-trivial change [not mere rewording] introduced as of this commit, in comparison to the table stated in the preceding commit): +=========+=========+=========+=========+=========+=========+=========+ #client(x)# libqb(a) usage # libqb(b) usage # # vvv #---------+---------+---------+---------+---------+---------+ # V # direct | libX(a) : libX(b) # direct | libX(a) : libX(b) # +=========+=========+=========+=========+=========+=========+=========+ # x = a # OK | OK : # | : # # x = b # | : # | : # +=========+=========+=========+=========+=========+=========+=========+ Everything is green \o/ Note: as of this fix, it is assumed that the non-green counterpart of this table in the message for the preceding commit doubles as an indicator how will mixing the logging participants wrt. linker+libqb version work out, when "X(Y)" becomes read as "X linked with linker Y under additional restriction on libqb version when compile-time link is performed of the particular part": X(a) .. ld.bfd < 2.29 OR [arbitrary ld.bfd AND libqb after this fix) X(b) .. ld.bfd = 2.29 (and likely on) AND libqb up to, but excluding this fix * * * Let's also state some imperfections and loops kept open: Deficiencies: * whenever anything is compiled against our install-time-modified libqb.so so as to force the visibility of the discussed symbols (or when compiling [with] libqb internally): > /usr/bin/ld: warning: ../lib/qblog_script.ld contains output sections; did you forget -T? - not solvable as long as we use the linker script, and there's hardly any other way not requiring the libqb consumers to adapt in any way Open questions: * what about binutils 2.99.1 partially mitigating the impact of the above commits * should we enable attribute((__section__)) for powerpc platforms? it seems to work well there -- and related to that, we need to figure out how static and dynamic call sites can live together -- say what will happen to logging programs compiled with libqb w/o attribute sections when the underlying libqb is flipped to the one with them... * * * [*] basically GCC's section("SECNAME") __attribute__ annotation of the global variables + linker described behaviour previously mistakenly taken for granted References: [1] http://oss.clusterlabs.org/pipermail/developers/2017-July/000503.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=1477354#c2 + comment 8 [3] https://bugzilla.redhat.com/show_bug.cgi?id=1477354#c9 Signed-off-by: Jan Pokorný --- configure.ac | 71 +++++++++++++++++++++++++++++++++++++++--------- lib/Makefile.am | 67 +++++++++++++++++++++++++++++++++++++++++++++ lib/log.c | 7 +++++ lib/qblog_script.la.in | 15 ++++++++++ lib/qblog_script.ld.in | 17 ++++++++++++ lib/qblog_script_noop.ld | 1 + 7 files changed, 165 insertions(+), 13 deletions(-) create mode 100644 lib/qblog_script.la.in create mode 100644 lib/qblog_script.ld.in create mode 100644 lib/qblog_script_noop.ld diff --git a/configure.ac b/configure.ac index 5601302..5d17443 100644 --- a/configure.ac +++ b/configure.ac @@ -618,31 +618,74 @@ AC_SUBST(HAVE_SLOW_TESTS) # --- callsite sections --- if test "x${GCC}" = xyes; then + LDFLAGS_save="${LDFLAGS}" AC_MSG_CHECKING([whether GCC supports __attribute__((section()) + ld supports orphan sections]) if test "x${ac_cv_link_attribute_section}" = x ; then - AC_TRY_RUN( - AC_LANG_PROGRAM( + LDFLAGS="${LDFLAGS_save} -shared -fPIC" + AC_LINK_IFELSE( + [AC_LANG_SOURCE( [[#include - extern void * __start___verbose, * __stop___verbose;]], - [[static int my_var __attribute__((section("__verbose"))) = 5; - assert(("non-empty data section", __start___verbose != __stop___verbose)); - assert(("no data section value loss", my_var == 5)); - ]]), - [gcc_has_attribute_section=yes], + extern int __start___verbose[], __stop___verbose[]; + int test(void) { + static int my_var __attribute__((section("__verbose"))) = 3; + assert(("L:non-empty data section", __start___verbose != __stop___verbose)); + assert(("L:no data section value loss", my_var == 3 /* for 2.29.1+ */)); + return *((int *) __start___verbose); + }]])], + [gcc_has_attribute_section=yes; cp "conftest${EXEEXT}" "lib_conftest.so"], [gcc_has_attribute_section=no] ) + LDFLAGS="${LDFLAGS_save}" else gcc_has_attribute_section=${ac_cv_link_attribute_section} fi - AC_MSG_RESULT($gcc_has_attribute_section) - if test $gcc_has_attribute_section = yes; then + + # in the failing case (e.g. with ld from binutils 2.29), it's likely the + # following will fail readily in linkage (hidden symbol `__stop___verbose' + # in conftest is referenced by DSO), but keep the sensible test (in-binary + # symbol is expected to be propagated into the library, and to draw the + # full circle back to binary through standard return value passing; + # -rpath passed because LD_LIBRARY_PATH exporting is unwieldy here); + # moreover, "my_var" == 3 assertion above is necessary so that binutils + # 2.29.1+ will not slip other parts of the overall is-workaround-needed + # harness, as it restored some (but not all) of the original behaviour, + # but the workaround is still provably needed + if test "x${gcc_has_attribute_section}" = xyes; then + AC_MSG_CHECKING([whether linker emits global boundary symbols for orphan sections]) + LDFLAGS="${LDFLAGS_save} -L. -l_conftest -Wl,-rpath=$(pwd)" + AC_TRY_RUN( + AC_LANG_PROGRAM( + [[#include + extern int __start___verbose[], __stop___verbose[]; + int test(void);]], + [[static int my_var __attribute__((section("__verbose"))) = 5; + assert(("P:non-empty data section", __start___verbose != __stop___verbose)); + assert(("P:no data section value loss", my_var == test() /*5?*/)); + ]]), + [gcc_has_attribute_section_visible=yes], + [gcc_has_attribute_section_visible=no] + ) + LDFLAGS="${LDFLAGS_save}" + AC_MSG_RESULT($gcc_has_attribute_section_visible) + rm -f "lib_conftest.so" + AC_DEFINE([QB_HAVE_ATTRIBUTE_SECTION], 1, [Enabling code using __attribute__((section))]) - PACKAGE_FEATURES="$PACKAGE_FEATURES attribute-section" AC_SUBST([client_dlopen_LIBS],[$dlopen_LIBS]) + if test "x${gcc_has_attribute_section_visible}" = xyes; then + PACKAGE_FEATURES="$PACKAGE_FEATURES attribute-section" + else + AC_DEFINE([QB_NEED_ATTRIBUTE_SECTION_WORKAROUND], 1, + [Enabling code using __attribute__((section))]) + PACKAGE_FEATURES="$PACKAGE_FEATURES attribute-section-workaround" + fi fi fi +AM_CONDITIONAL(HAVE_GCC_ATTRIBUTE_SECTION, [test "x${gcc_has_attribute_section}" = xyes]) +AM_CONDITIONAL(NEED_GCC_ATTRIBUTE_SECTION_WORKAROUND, + [test "x${gcc_has_attribute_section}" = xyes \ + && test "x${gcc_has_attribute_section_visible}" != xyes]) # --- ansi --- if test "x${enable_ansi}" = xyes && \ @@ -724,9 +767,11 @@ AC_CONFIG_FILES([Makefile docs/Makefile docs/common.dox docs/html.dox - docs/man.dox]) + docs/man.dox + lib/qblog_script.la:lib/qblog_script.la.in]) -AC_CONFIG_LINKS([tests/functional/GNUmakefile:tests/functional/GNUmakefile]) +AC_CONFIG_LINKS([lib/qblog_script_noop.ld:lib/qblog_script_noop.ld + tests/functional/GNUmakefile:tests/functional/GNUmakefile]) AC_OUTPUT diff --git a/lib/Makefile.am b/lib/Makefile.am index 0bebeb5..cc7417c 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -19,6 +19,7 @@ # along with libqb. If not, see . +CLEANFILES = qblog_script.ld MAINTAINERCLEANFILES = Makefile.in noinst_HEADERS = ipc_int.h util_int.h ringbuffer_int.h loop_int.h \ @@ -39,9 +40,48 @@ source_to_lint = util.c hdb.c ringbuffer.c ringbuffer_helper.c \ log_syslog.c log_dcs.c log_format.c \ map.c skiplist.c hashtable.c trie.c +# Following two files related to linkage using classic ld from binutils 2.29+ +# with which we cannot afford to lose public access to section boundary symbols +# (as the mentioned version started to scope them privately by default, see +# the comment within the first of the files, ultimately leading to broken +# logging functionality of libqb) deserve a bit of explanation: +# * qblog_script.ld +# - linker script that instructs the output section that those symbols should +# be visible, i.e. supports the same behaviour regardless of ld version +# - serves two purposes: +# . local: libqb itself and its "private" (cf. examples) users need those +# symbols visible, which is achieved with a help of the other file +# . system-wide: whenever the non-private library users link against libqb +# (it's development files), this linker script with +# prepended INPUT command so as to refer to the actual +# libqb library (it's numbered alias that is eventually +# resolved to proper shared library) is masked as libqb.so, +# this arrangement achieves the libqb's user will have +# the discussed symbols visible alike +# * qblog_script.la +# - as mentioned earlier, this indirectly hooks into libtool machinery, with +# the only true intention of injecting "-Wl," +# into "inherited_linker_flags" libtool archive variable, from where it's +# subsequently spread into the build process of all the internal library +# users, assuming they have their dep arranged as "user_LIBADD=libqb.la" +# (this also alleviates the burden on getting things right if, e.g., any +# libqb user consumes it directly like this from its own sub-checkout tree) +# - it indirectly, once libtool prechew the original link command +# originally referring to this file, it turns such reference into the +# "real" library reference (here combining libdir and old_library +# variables within the file), also ensures libqb itself will visibly +# expose the discussed symbols, because such references point again to +# the (not enriched) linker script file that will get interpreted just +# like that during the last build step of the library +EXTRA_libqb_la_DEPENDENCIES = qblog_script.ld qblog_script.la +EXTRA_DIST = qblog_script.ld.in qblog_script.la.in qblog_script_noop.ld + libqb_la_SOURCES = $(source_to_lint) unix.c libqb_la_CFLAGS = $(PTHREAD_CFLAGS) libqb_la_LIBADD = $(LTLIBOBJS) $(dlopen_LIBS) $(PTHREAD_LIBS) $(socket_LIBS) +if NEED_GCC_ATTRIBUTE_SECTION_WORKAROUND +libqb_la_LIBADD += qblog_script.la +endif AM_LDFLAGS = $(LDFLAGS_COPY:-Bsymbolic-functions=) @@ -60,6 +100,33 @@ else endif endif +qblog_script.ld: %.ld: %.ld.in + $(AM_V_GEN)$(CPP) -xc -I$(top_srcdir)/include -C -P $< \ + | sed -n "/$$(sed -n '/^[^#]/{s/[*\/]/\\\0/g;p;q}' $<)/{:r;p;n;br}" \ + > $@ + +# find the libqb.so symlink's target, if so, try to find out, iteratively, +# its gradually shorter forms that likewise symlinks the same target as the +# original libqb.so path, point to that file from the linker script using +# qblog_script.ld as a template, storing result in place of original libqb.so +# (e.g., libqb.so := "INPUT(libqb.so.0) " [...] "SECTIONS { " [...] "}") +# NOTE: readlink nor realpath are POSIX; not chained links ready +# NOTE: conservative check, i.e., not per NEED_GCC_ATTRIBUTE_SECTION_WORKAROUND +if HAVE_GCC_ATTRIBUTE_SECTION +install-exec-hook: qblog_script.ld + target=$$(ls -l "$(DESTDIR)$(libdir)/libqb.so" || :); \ + target=$${target#* -> }; t1_bn=$$(basename "$${target}" || :); \ + while test -n "$${t1_bn}"; do t2_bn=$${t1_bn%.*[0-9]*}; \ + test "$${t2_bn}" != libqb.so || break; \ + test -L "$${t2_bn}" || { t1_bn=$${t2_bn}; continue; }; \ + t2_target=$$(ls -l "$${t2_bn}" || break); t2_target=$${t2_target#* -> }; \ + test "$${t2_target}" = "$${target}" || break; \ + t1_bn=$${t2_bn}; done; test -n "$${t1_bn}" || \ + { echo "only applicable to SO symlink scheme"; exit 1; }; \ + echo "INPUT($${t1_bn})" > "$(DESTDIR)$(libdir)/libqb.so-t" + cat $< >> "$(DESTDIR)$(libdir)/libqb.so-t" + mv -f "$(DESTDIR)$(libdir)/libqb.so-t" "$(DESTDIR)$(libdir)/libqb.so" +endif pkgconfigdir = $(libdir)/pkgconfig pkgconfig_DATA = libqb.pc diff --git a/lib/log.c b/lib/log.c index bfd218f..4ed432f 100644 --- a/lib/log.c +++ b/lib/log.c @@ -40,6 +40,13 @@ #include "util_int.h" #include +#if defined(QB_NEED_ATTRIBUTE_SECTION_WORKAROUND) && !defined(S_SPLINT_S) +/* following only needed to force these symbols be global + with ld 2.29: https://bugzilla.redhat.com/1477354 */ +struct qb_log_callsite __attribute__((weak)) QB_ATTR_SECTION_START[] = { 0 }; +struct qb_log_callsite __attribute__((weak)) QB_ATTR_SECTION_STOP[] = { 0 }; +#endif + static struct qb_log_target conf[QB_LOG_TARGET_MAX]; static uint32_t conf_active_max = 0; static int32_t in_logger = QB_FALSE; diff --git a/lib/qblog_script.la.in b/lib/qblog_script.la.in new file mode 100644 index 0000000..f262df8 --- /dev/null +++ b/lib/qblog_script.la.in @@ -0,0 +1,15 @@ +# Generated by libtool +# NOTE: above line is just to pass func_ltwrapper_script_p sanity check of +# libtool script, as we are basically sort of abusing it so as to inject +# our custom linker script to "private" (cf. examples) users of libqb.la + +# shall rather carry a location of old_library (possibly libdir or something +# else, but installed=no needed to suppress 'library moved' warning then) as +# it's together (with libtool implied prefix otherwise) used for linking libqb +libdir=@abs_builddir@ + +# avoids issues with library_names (spurious rpath emitting, relink-on-install) +old_library=qblog_script_noop.ld + +# subject of our injection into libqb.la impacting build time for private users +inherited_linker_flags=-Wl,@abs_builddir@/qblog_script.ld diff --git a/lib/qblog_script.ld.in b/lib/qblog_script.ld.in new file mode 100644 index 0000000..1f37976 --- /dev/null +++ b/lib/qblog_script.ld.in @@ -0,0 +1,17 @@ +#include +/* GNU ld script + This atypical arrangement enforces global visibility of boundary symbols + (QB_ATTR_SECTION_START, QB_ATTR_SECTION_STOP) for the custom section + QB_ATTR_SECTION used for compile-time offloading of the logging call sites + tracking. While libqb relies on these being global, default linker from + binutils change the visibility as of version 2.29, making the logging + unusable without artificial stimulus: https://bugzilla.redhat.com/1477354 */ +SECTIONS { +#ifdef QB_HAVE_ATTRIBUTE_SECTION + QB_ATTR_SECTION : { + QB_ATTR_SECTION_START = .; + *(QB_ATTR_SECTION); + QB_ATTR_SECTION_STOP = .; + } +#endif +} diff --git a/lib/qblog_script_noop.ld b/lib/qblog_script_noop.ld new file mode 100644 index 0000000..f037fca --- /dev/null +++ b/lib/qblog_script_noop.ld @@ -0,0 +1 @@ +/* this is an empty linker script having a role of a NO-OP link object */ -- 2.14.2