[PATCH 0/4] build fixes and improvements

[PATCH 0/4] build fixes and improvements

[Jani Nikula]

I was looking into the Meson build system [1], and tried converting
notmuch to use it as a learning experience. I'm not sure if that'll lead
anywhere, but I noted some issues in the current build while at it.

The first two are genuine fixes, the last two I think make the build
easier to understand, but also make it more straightforward to convert
the build to Meson.

BR,
Jani.


Jani Nikula (4):
  build: do not export compat functions from lib
  compat: don't include compat.h from the feature test source
  build: switch to hiding libnotmuch symbols by default
  build: visibility=default for library structs is no longer needed

 .gitignore                 |  1 -
 Makefile.local             |  1 +
 compat/have_timegm.c       |  1 -
 lib/Makefile.local         | 25 ++++++++++++-------------
 lib/database-private.h     |  4 ----
 lib/gen-version-script.sh  | 29 -----------------------------
 lib/message.cc             |  2 +-
 lib/notmuch-private.h      | 14 ++------------
 lib/notmuch.h              |  4 ++++
 lib/notmuch.sym            |  7 +++++++
 lib/query.cc               |  2 +-
 lib/thread.cc              |  2 +-
 test/T360-symbol-hiding.sh |  2 +-
 13 files changed, 30 insertions(+), 64 deletions(-)
 delete mode 100644 lib/gen-version-script.sh
 create mode 100644 lib/notmuch.sym

-- 
2.11.0

[PATCH 1/4] build: do not export compat functions from lib

[Jani Nikula]

Commits 9db214527213 ("lib/gen-version-script.h: add getline and
getdelim to notmuch.sym if needed") and 3242e29e57ac ("build: add
canonicalize_file_name to symbols exported from libnotmuch.so")
started exporting compat functions from libnotmuch so that the cli
could use them. But we shouldn't export such functions from the
library. They are not part of our ABI. Instead, the cli should include
its own copies of the compat functions.
---
 Makefile.local            | 1 +
 lib/gen-version-script.sh | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.local b/Makefile.local
index 3d3474e0c97e..6bc78ef8e969 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -212,6 +212,7 @@ dataclean: distclean
 	rm -rf $(DATACLEAN)
 
 notmuch_client_srcs =		\
+	$(notmuch_compat_srcs)	\
 	command-line-arguments.c\
 	debugger.c		\
 	status.c		\
diff --git a/lib/gen-version-script.sh b/lib/gen-version-script.sh
index 5621f2a9fd85..c98a07b0c701 100644
--- a/lib/gen-version-script.sh
+++ b/lib/gen-version-script.sh
@@ -24,6 +24,5 @@ while read sym; do
 	    ;;
     esac
 done
-nm $* | awk '$1 ~ "^[0-9a-fA-F][0-9a-fA-F]*$" && $2 == "T" && $3 ~ "^(getline|getdelim|canonicalize_file_name)$" {print $3 ";"}'
 sed  -n 's/^[[:space:]]*\(notmuch_[a-z_]*\)[[:space:]]*(.*/ \1;/p' $HEADER
 printf "local: *;\n};\n"
-- 
2.11.0

[PATCH 2/4] compat: don't include compat.h from the feature test source

[Jani Nikula]

The feature test code should test the build environment, and none of
the compat code should interfere with that. Don't include compat.h
from the feature test source. There should be no functional changes
here, but this is just the right thing to do.
---
 compat/have_timegm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/compat/have_timegm.c b/compat/have_timegm.c
index b62b7937feab..483fc3b6685d 100644
--- a/compat/have_timegm.c
+++ b/compat/have_timegm.c
@@ -1,5 +1,4 @@
 #include <time.h>
-#include "compat.h"
 
 int main()
 {
-- 
2.11.0

[PATCH 3/4] build: switch to hiding libnotmuch symbols by default

[Jani Nikula]

The dynamic generation of the linker version script for libnotmuch
exports has grown rather complicated.

Reverse the visibility control by hiding symbols by default using
-fvisibility=hidden, and explicitly exporting symbols in notmuch.h
using #pragma GCC visibility. (We could also use __attribute__
((visibility ("default"))) for each exported function, but the pragma
is more convenient.)

The above is not quite enough alone, as it would "leak" a number of
weak symbols from Xapian and C++ standard library. Combine it with a
small static version script that filters out everything except the
notmuch_* symbols that we explicitly exposed, and the C++ RTTI
typeinfo symbols for exception handling.

Finally, as the symbol hiding test can no longer look at the generated
symbol table, switch the test to parse the functions from notmuch.h.
---
 .gitignore                 |  1 -
 lib/Makefile.local         | 25 ++++++++++++-------------
 lib/database-private.h     |  4 ----
 lib/gen-version-script.sh  | 28 ----------------------------
 lib/notmuch-private.h      |  4 ----
 lib/notmuch.h              |  4 ++++
 lib/notmuch.sym            |  7 +++++++
 test/T360-symbol-hiding.sh |  2 +-
 8 files changed, 24 insertions(+), 51 deletions(-)
 delete mode 100644 lib/gen-version-script.sh
 create mode 100644 lib/notmuch.sym

diff --git a/.gitignore b/.gitignore
index 296030c76977..7b283fb3bff2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,7 +7,6 @@ tags
 *cscope*
 .deps
 /notmuch
-notmuch.sym
 notmuch-shared
 libnotmuch.so*
 libnotmuch*.dylib
diff --git a/lib/Makefile.local b/lib/Makefile.local
index d36fd5a0678e..bf6e06494748 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -1,5 +1,12 @@
 # -*- makefile -*-
 
+dir := lib
+
+# The (often-reused) $dir works fine within targets/prerequisites,
+# but cannot be used reliably within commands, so copy its value to a
+# variable that is not reused.
+lib := $(dir)
+
 ifeq ($(PLATFORM),MACOSX)
 LIBRARY_SUFFIX = dylib
 # On OS X, library version numbers go before suffix.
@@ -12,7 +19,7 @@ LIBRARY_SUFFIX = so
 LINKER_NAME = libnotmuch.$(LIBRARY_SUFFIX)
 SONAME = $(LINKER_NAME).$(LIBNOTMUCH_VERSION_MAJOR)
 LIBNAME = $(SONAME).$(LIBNOTMUCH_VERSION_MINOR).$(LIBNOTMUCH_VERSION_RELEASE)
-LIBRARY_LINK_FLAG = -shared -Wl,--version-script=notmuch.sym,-soname=$(SONAME) $(NO_UNDEFINED_LDFLAGS)
+LIBRARY_LINK_FLAG = -shared -Wl,--version-script=$(lib)/notmuch.sym,-soname=$(SONAME) $(NO_UNDEFINED_LDFLAGS)
 ifeq ($(PLATFORM),OPENBSD)
 LIBRARY_LINK_FLAG += -lc
 endif
@@ -23,13 +30,8 @@ endif
 endif
 endif
 
-dir := lib
-extra_cflags += -I$(srcdir)/$(dir) -fPIC
-
-# The (often-reused) $dir works fine within targets/prerequisites,
-# but cannot be used reliably within commands, so copy its value to a
-# variable that is not reused.
-lib := $(dir)
+extra_cflags += -I$(srcdir)/$(dir) -fPIC -fvisibility=hidden
+extra_cxxflags += -fvisibility-inlines-hidden
 
 libnotmuch_c_srcs =		\
 	$(notmuch_compat_srcs)	\
@@ -60,12 +62,9 @@ libnotmuch_modules := $(libnotmuch_c_srcs:.c=.o) $(libnotmuch_cxx_srcs:.cc=.o)
 $(dir)/libnotmuch.a: $(libnotmuch_modules)
 	$(call quiet,AR) rcs $@ $^
 
-$(dir)/$(LIBNAME): $(libnotmuch_modules) notmuch.sym util/libnotmuch_util.a parse-time-string/libparse-time-string.a
+$(dir)/$(LIBNAME): $(libnotmuch_modules) util/libnotmuch_util.a parse-time-string/libparse-time-string.a
 	$(call quiet,CXX $(CXXFLAGS)) $(libnotmuch_modules) $(FINAL_LIBNOTMUCH_LDFLAGS) $(LIBRARY_LINK_FLAG) -o $@ util/libnotmuch_util.a parse-time-string/libparse-time-string.a
 
-notmuch.sym: $(srcdir)/$(dir)/notmuch.h $(libnotmuch_modules)
-	sh $(srcdir)/$(lib)/gen-version-script.sh $< $(libnotmuch_modules) > $@
-
 $(dir)/$(SONAME): $(dir)/$(LIBNAME)
 	ln -sf $(LIBNAME) $@
 
@@ -85,5 +84,5 @@ install-$(dir): $(dir)/$(LIBNAME)
 
 SRCS  := $(SRCS) $(libnotmuch_c_srcs) $(libnotmuch_cxx_srcs)
 CLEAN += $(libnotmuch_modules) $(dir)/$(SONAME) $(dir)/$(LINKER_NAME)
-CLEAN += $(dir)/$(LIBNAME) $(dir)/libnotmuch.a notmuch.sym
+CLEAN += $(dir)/$(LIBNAME) $(dir)/libnotmuch.a
 CLEAN += $(dir)/notmuch.h.gch
diff --git a/lib/database-private.h b/lib/database-private.h
index ab3d9691247f..727b1d616f24 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -38,8 +38,6 @@
 
 #include <xapian.h>
 
-#pragma GCC visibility push(hidden)
-
 /* Bit masks for _notmuch_database::features.  Features are named,
  * independent aspects of the database schema.
  *
@@ -248,6 +246,4 @@ _notmuch_database_get_terms_with_prefix (void *ctx, Xapian::TermIterator &i,
 					 Xapian::TermIterator &end,
 					 const char *prefix);
 
-#pragma GCC visibility pop
-
 #endif
diff --git a/lib/gen-version-script.sh b/lib/gen-version-script.sh
deleted file mode 100644
index c98a07b0c701..000000000000
--- a/lib/gen-version-script.sh
+++ /dev/null
@@ -1,28 +0,0 @@
-set -eu
-
-# we go through a bit of work to get the unmangled names of the
-# typeinfo symbols because of
-# https://sourceware.org/bugzilla/show_bug.cgi?id=10326
-
-if [ $# -lt 2 ]; then
-    echo Usage: $0 header obj1 obj2 obj3
-    exit 1;
-fi
-
-HEADER=$1
-shift
-
-printf '{\nglobal:\n'
-nm  $* | awk '$1 ~ "^[0-9a-fA-F][0-9a-fA-F]*$" && $3 ~ "Xapian.*Error" {print $3}' | sort | uniq | \
-while read sym; do
-    demangled=$(c++filt $sym)
-    case $demangled in
-	typeinfo*)
-	    printf "\t$sym;\n"
-	    ;;
-	*)
-	    ;;
-    esac
-done
-sed  -n 's/^[[:space:]]*\(notmuch_[a-z_]*\)[[:space:]]*(.*/ \1;/p' $HEADER
-printf "local: *;\n};\n"
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 8587e86ca57a..926707d82d12 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -52,8 +52,6 @@ NOTMUCH_BEGIN_DECLS
 #include "error_util.h"
 #include "string-util.h"
 
-#pragma GCC visibility push(hidden)
-
 #ifdef DEBUG
 # define DEBUG_DATABASE_SANITY 1
 # define DEBUG_QUERY 1
@@ -621,6 +619,4 @@ _notmuch_talloc_steal (const void *new_ctx, const T *ptr)
 #endif
 #endif
 
-#pragma GCC visibility pop
-
 #endif
diff --git a/lib/notmuch.h b/lib/notmuch.h
index d374dc960fe6..e17454442333 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -43,6 +43,8 @@ NOTMUCH_BEGIN_DECLS
 
 #include <time.h>
 
+#pragma GCC visibility push(default)
+
 #ifndef FALSE
 #define FALSE 0
 #endif
@@ -2120,6 +2122,8 @@ notmuch_bool_t
 notmuch_built_with (const char *name);
 /* @} */
 
+#pragma GCC visibility pop
+
 NOTMUCH_END_DECLS
 
 #endif
diff --git a/lib/notmuch.sym b/lib/notmuch.sym
new file mode 100644
index 000000000000..7d0c0af40ad9
--- /dev/null
+++ b/lib/notmuch.sym
@@ -0,0 +1,7 @@
+{
+global:
+	_ZTI*;
+	_ZTS*;
+	notmuch_*;
+local: *;
+};
diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
index b3dbb1b543e9..9c6d4e647db5 100755
--- a/test/T360-symbol-hiding.sh
+++ b/test/T360-symbol-hiding.sh
@@ -27,7 +27,7 @@ test_expect_equal "$result" "$output"
 
 test_begin_subtest 'comparing existing to exported symbols'
 nm -P $TEST_DIRECTORY/../lib/libnotmuch.so | awk '$2 == "T" && $1 ~ "^notmuch" {print $1}' | sort | uniq > ACTUAL
-sed -n 's/[[:blank:]]*\(notmuch_[^;]*\);/\1/p' $TEST_DIRECTORY/../notmuch.sym | sort | uniq > EXPORTED
+sed -n 's/^\(notmuch_[a-zA-Z0-9_]*\)[[:blank:]]*(.*/\1/p' $TEST_DIRECTORY/../lib/notmuch.h | sort | uniq > EXPORTED
 test_expect_equal_file EXPORTED ACTUAL
 
 test_done
-- 
2.11.0

[PATCH 4/4] build: visibility=default for library structs is no longer needed

[Jani Nikula]

Commit d5523ead90b6 ("Mark some structures in the library interface
with visibility=default attribute.") fixed some mixed visibility
issues with structs. With the symbol default visibility reversed, this
is no longer a problem.
---
 lib/message.cc        |  2 +-
 lib/notmuch-private.h | 10 ++--------
 lib/query.cc          |  2 +-
 lib/thread.cc         |  2 +-
 4 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index c2721191043e..b330dcce5ecf 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -26,7 +26,7 @@
 
 #include <gmime/gmime.h>
 
-struct visible _notmuch_message {
+struct _notmuch_message {
     notmuch_database_t *notmuch;
     Xapian::docid doc_id;
     int frozen;
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 926707d82d12..ac315e4c99b7 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -74,12 +74,6 @@ NOTMUCH_BEGIN_DECLS
 
 #define unused(x) x __attribute__ ((unused))
 
-#ifdef __cplusplus
-# define visible __attribute__((visibility("default")))
-#else
-# define visible
-#endif
-
 /* Thanks to Andrew Tridgell's (SAMBA's) talloc for this definition of
  * unlikely. The talloc source code comes to us via the GNU LGPL v. 3.
  */
@@ -453,7 +447,7 @@ typedef struct _notmuch_message_list {
  * somewhere with some nasty C++ objects in it. We'll try to maintain
  * ignorance of that here. (See notmuch_mset_messages_t in query.cc)
  */
-struct visible _notmuch_messages {
+struct _notmuch_messages {
     notmuch_bool_t is_of_list_type;
     notmuch_doc_id_set_t *excluded_doc_ids;
     notmuch_message_node_t *iterator;
@@ -522,7 +516,7 @@ typedef struct _notmuch_string_node {
     struct _notmuch_string_node *next;
 } notmuch_string_node_t;
 
-typedef struct visible _notmuch_string_list {
+typedef struct _notmuch_string_list {
     int length;
     notmuch_string_node_t *head;
     notmuch_string_node_t **tail;
diff --git a/lib/query.cc b/lib/query.cc
index 212e27f0bc8e..9c6ecc8db5ce 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -49,7 +49,7 @@ struct _notmuch_doc_id_set {
 #define DOCIDSET_WORD(bit) ((bit) / CHAR_BIT)
 #define DOCIDSET_BIT(bit) ((bit) % CHAR_BIT)
 
-struct visible _notmuch_threads {
+struct _notmuch_threads {
     notmuch_query_t *query;
 
     /* The ordered list of doc ids matched by the query. */
diff --git a/lib/thread.cc b/lib/thread.cc
index 561ca5bec783..1a1ecfa5507e 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -26,7 +26,7 @@
 
 #define EMPTY_STRING(s) ((s)[0] == '\0')
 
-struct visible _notmuch_thread {
+struct _notmuch_thread {
     notmuch_database_t *notmuch;
     char *thread_id;
     char *subject;
-- 
2.11.0