Обсуждение: Use -fvisibility=hidden for shared libraries

Hi,

Currently postgres builds extension shared libraries (i.e. pretty much
everything but libpq) with global visibilty. I.e. symbols that are not
static will be visible from outside the shared library.

On the one platform where that behaviour is not available, namely
windows, we emulate it by analyzing the input files to the shared
library and exporting all the symbols therein.

For the meson build [1] proposal that turned out to be a bit more
verbose to implement than I'd liked. Thinking about the problem I
realized that, at least I think so, there's really no good reason to
force-export symbols in our shared libraries:

Because the number of symbols required from shared libraries is
typically small, and the majority of the symbols are generated via
PG_FUNCTION_INFO_V1, it isn't a lot of work to explicitly export the
necessary symbols.


I think this is a much more feasible proposal than, as has been
suggested in the past, using explicit visibility for the entire
backend. The amount of functions that'd need to be annotated in the
backend is huge, and I don't think we have a reasonable starting point
for limiting what we'd export.


There are a few extension libs that need manual export
annotations. These are libraries that are not just referenced by the
backend, but also from other extensions, e.g. for transforms.


The patch e.g. reduces the number of exported symbols for

- plpython: 61 -> 29
- plperl: 32 -> 17
- llvmjit: 84 -> 5
- postgres_fdw: 48 -> 15
- dict_snowball: 182 -> 8

Besides the smaller number of symbols (which can impact library load
times a bit, although the numbers here are likely small enough to not
matter), using -fvisibility=hidden also can improve code generation,
because it allows the compiler & linker to generate a plain function
call, rather than having to go through the elf symbol-interposition
table.


The attached patch is a prototype of this idea.

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/20211012083721.hvixq4pnh2pixr3j%40alap3.anarazel.de

On Sun, 2021-10-31 at 19:03 -0700, Andres Freund wrote:
> Currently postgres builds extension shared libraries (i.e. pretty much
> everything but libpq) with global visibilty. I.e. symbols that are not
> static will be visible from outside the shared library.
> 
> On the one platform where that behaviour is not available, namely
> windows, we emulate it by analyzing the input files to the shared
> library and exporting all the symbols therein.
> 
> For the meson build [1] proposal that turned out to be a bit more
> verbose to implement than I'd liked. Thinking about the problem I
> realized that, at least I think so, there's really no good reason to
> force-export symbols in our shared libraries:
> 
> Because the number of symbols required from shared libraries is
> typically small, and the majority of the symbols are generated via
> PG_FUNCTION_INFO_V1, it isn't a lot of work to explicitly export the
> necessary symbols.

That sounds like a good idea.

I see that at least clang and gcc support this flag.

Could the reduced number of exported functions be a problem, if someone
relies on some function being exported?  It may not be worth worrying about,
and they can always come and make a case for that symbol to be exported.

Yours,
Laurenz Albe

Hi,

On 2021-11-01 05:55:40 +0100, Laurenz Albe wrote:
> I see that at least clang and gcc support this flag.

Yes - and have for a long time. 2004 or so.


> Could the reduced number of exported functions be a problem, if someone
> relies on some function being exported?  It may not be worth worrying about,
> and they can always come and make a case for that symbol to be exported.

Hm. Possible, but I don't think common. It's pretty rare to need symbols
from a postgres extension shared library from another shared
library. The most common case are transforms. To add a new transform
one, from what I've seen, needs to expose previously internal stuff from
an extension anyway... And transforms aren't all that common.

And yes, we can easily just add a few additional exports over time.


One nice advantage of this is that the !windows build behaves more
similar to the windows case, which makes it easier to detect build
breakages locally...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> [ v2-0001-Use-hidden-visibility-for-shared-libraries-where-.patch ]

This seems like a good idea, but it's failing to apply right now,
mainly because of some cleanup I did in e04a8059a.  As far as I can
tell given the now-clarified meanings of PGDLLIMPORT/PGDLLEXPORT,
this patch shouldn't be touching PGDLLIMPORT.  The attached revision
works for me under gcc 8.5 and clang 13.

Also, it seemed like you'd maybe been more enthusiastic than necessary
about plastering PGDLLEXPORT on things.  I got through check-world
cleanly without the diffs in either ltree.h or worker_spi.c (although
I confess being confused why I didn't need the latter).  I didn't try
individually removing other diffs.  Those diffs are still in v3 below,
but we should clarify exactly which functions need marking.

            regards, tom lane

diff --git a/configure b/configure
index 3f2aea0d7d..98eabe8b08 100755
--- a/configure
+++ b/configure
@@ -735,6 +735,8 @@ CPP
 CFLAGS_SL
 BITCODE_CXXFLAGS
 BITCODE_CFLAGS
+CXXFLAGS_SL_MOD
+CFLAGS_SL_MOD
 CFLAGS_VECTORIZE
 CFLAGS_UNROLL_LOOPS
 PERMIT_DECLARATION_AFTER_STATEMENT
@@ -6288,6 +6290,154 @@ if test x"$pgac_cv_prog_CC_cflags__ftree_vectorize" = x"yes"; then
 fi


+  #
+  # If the compiler knows how to hide symbols, set CFLAGS_SL_MOD
+  # to the switch needed for that, and define HAVE_VISIBILITY_ATTRIBUTE.
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -fvisibility=hidden, for CFLAGS_SL_MOD" >&5
+$as_echo_n "checking whether ${CC} supports -fvisibility=hidden, for CFLAGS_SL_MOD... " >&6; }
+if ${pgac_cv_prog_CC_cflags__fvisibility_hidden+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS_SL_MOD} -fvisibility=hidden"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__fvisibility_hidden=yes
+else
+  pgac_cv_prog_CC_cflags__fvisibility_hidden=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__fvisibility_hidden" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__fvisibility_hidden" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__fvisibility_hidden" = x"yes"; then
+  CFLAGS_SL_MOD="${CFLAGS_SL_MOD} -fvisibility=hidden"
+fi
+
+
+  if test "$pgac_cv_prog_CC_cflags__fvisibility_hidden" = yes; then
+
+$as_echo "#define HAVE_VISIBILITY_ATTRIBUTE 1" >>confdefs.h
+
+  fi
+  # For C++ we additionally want -fvisibility-inlines-hidden
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -fvisibility=hidden, for CXXFLAGS_SL_MOD"
>&5
+$as_echo_n "checking whether ${CXX} supports -fvisibility=hidden, for CXXFLAGS_SL_MOD... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__fvisibility_hidden+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS_SL_MOD} -fvisibility=hidden"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  pgac_cv_prog_CXX_cxxflags__fvisibility_hidden=yes
+else
+  pgac_cv_prog_CXX_cxxflags__fvisibility_hidden=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__fvisibility_hidden" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__fvisibility_hidden" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__fvisibility_hidden" = x"yes"; then
+  CXXFLAGS_SL_MOD="${CXXFLAGS_SL_MOD} -fvisibility=hidden"
+fi
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -fvisibility-inlines-hidden, for
CXXFLAGS_SL_MOD">&5 
+$as_echo_n "checking whether ${CXX} supports -fvisibility-inlines-hidden, for CXXFLAGS_SL_MOD... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS_SL_MOD} -fvisibility-inlines-hidden"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden=yes
+else
+  pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" = x"yes"; then
+  CXXFLAGS_SL_MOD="${CXXFLAGS_SL_MOD} -fvisibility-inlines-hidden"
+fi
+
   #
   # The following tests want to suppress various unhelpful warnings by adding
   # -Wno-foo switches.  But gcc won't complain about unrecognized -Wno-foo
@@ -6940,6 +7090,8 @@ fi



+
+
 # Determine flags used to emit bitcode for JIT inlining.
 # 1. We must duplicate any behaviour-changing compiler flags used above,
 # to keep compatibility with the compiler used for normal Postgres code.
diff --git a/configure.ac b/configure.ac
index 95287705f6..b39d9c64f4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -523,6 +523,17 @@ if test "$GCC" = yes -a "$ICC" = no; then
   # Optimization flags for specific files that benefit from vectorization
   PGAC_PROG_CC_VAR_OPT(CFLAGS_VECTORIZE, [-ftree-vectorize])
   #
+  # If the compiler knows how to hide symbols, set CFLAGS_SL_MOD
+  # to the switch needed for that, and define HAVE_VISIBILITY_ATTRIBUTE.
+  PGAC_PROG_CC_VAR_OPT(CFLAGS_SL_MOD, [-fvisibility=hidden])
+  if test "$pgac_cv_prog_CC_cflags__fvisibility_hidden" = yes; then
+     AC_DEFINE([HAVE_VISIBILITY_ATTRIBUTE], 1,
+               [Define to 1 if your compiler knows the visibility("hidden") attribute.])
+  fi
+  # For C++ we additionally want -fvisibility-inlines-hidden
+  PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MOD, [-fvisibility=hidden])
+  PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MOD, [-fvisibility-inlines-hidden])
+  #
   # The following tests want to suppress various unhelpful warnings by adding
   # -Wno-foo switches.  But gcc won't complain about unrecognized -Wno-foo
   # switches, so we have to test for the positive form and if that works,
@@ -576,6 +587,8 @@ fi

 AC_SUBST(CFLAGS_UNROLL_LOOPS)
 AC_SUBST(CFLAGS_VECTORIZE)
+AC_SUBST(CFLAGS_SL_MOD)
+AC_SUBST(CXXFLAGS_SL_MOD)

 # Determine flags used to emit bitcode for JIT inlining.
 # 1. We must duplicate any behaviour-changing compiler flags used above,
diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
index bf4a565ed9..625134c9f6 100644
--- a/contrib/hstore/hstore.h
+++ b/contrib/hstore/hstore.h
@@ -147,7 +147,7 @@ typedef struct
     } while (0)

 /* DatumGetHStoreP includes support for reading old-format hstore values */
-extern HStore *hstoreUpgrade(Datum orig);
+extern PGDLLEXPORT HStore *hstoreUpgrade(Datum orig);

 #define DatumGetHStoreP(d) hstoreUpgrade(d)

@@ -168,14 +168,14 @@ typedef struct
     bool        needfree;        /* need to pfree the value? */
 } Pairs;

-extern int    hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen);
-extern HStore *hstorePairs(Pairs *pairs, int32 pcount, int32 buflen);
+extern PGDLLEXPORT int    hstoreUniquePairs(Pairs *a, int32 l, int32 *buflen);
+extern PGDLLEXPORT HStore *hstorePairs(Pairs *pairs, int32 pcount, int32 buflen);

-extern size_t hstoreCheckKeyLen(size_t len);
-extern size_t hstoreCheckValLen(size_t len);
+extern PGDLLEXPORT size_t hstoreCheckKeyLen(size_t len);
+extern PGDLLEXPORT size_t hstoreCheckValLen(size_t len);

-extern int    hstoreFindKey(HStore *hs, int *lowbound, char *key, int keylen);
-extern Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);
+extern PGDLLEXPORT int    hstoreFindKey(HStore *hs, int *lowbound, char *key, int keylen);
+extern PGDLLEXPORT Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);

 #define HStoreContainsStrategyNumber    7
 #define HStoreExistsStrategyNumber        9
@@ -194,7 +194,7 @@ extern Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);
 #if HSTORE_POLLUTE_NAMESPACE
 #define HSTORE_POLLUTE(newname_,oldname_) \
     PG_FUNCTION_INFO_V1(oldname_);          \
-    Datum newname_(PG_FUNCTION_ARGS);      \
+    extern PGDLLEXPORT Datum newname_(PG_FUNCTION_ARGS);      \
     Datum oldname_(PG_FUNCTION_ARGS) { return newname_(fcinfo); } \
     extern int no_such_variable
 #else
diff --git a/contrib/ltree/ltree.h b/contrib/ltree/ltree.h
index 5b4be5e680..d8bcdedbdb 100644
--- a/contrib/ltree/ltree.h
+++ b/contrib/ltree/ltree.h
@@ -176,30 +176,30 @@ typedef struct


 /* use in array iterator */
-Datum        ltree_isparent(PG_FUNCTION_ARGS);
-Datum        ltree_risparent(PG_FUNCTION_ARGS);
-Datum        ltq_regex(PG_FUNCTION_ARGS);
-Datum        ltq_rregex(PG_FUNCTION_ARGS);
-Datum        lt_q_regex(PG_FUNCTION_ARGS);
-Datum        lt_q_rregex(PG_FUNCTION_ARGS);
-Datum        ltxtq_exec(PG_FUNCTION_ARGS);
-Datum        ltxtq_rexec(PG_FUNCTION_ARGS);
-Datum        _ltq_regex(PG_FUNCTION_ARGS);
-Datum        _ltq_rregex(PG_FUNCTION_ARGS);
-Datum        _lt_q_regex(PG_FUNCTION_ARGS);
-Datum        _lt_q_rregex(PG_FUNCTION_ARGS);
-Datum        _ltxtq_exec(PG_FUNCTION_ARGS);
-Datum        _ltxtq_rexec(PG_FUNCTION_ARGS);
-Datum        _ltree_isparent(PG_FUNCTION_ARGS);
-Datum        _ltree_risparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltree_isparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltree_risparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltq_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltq_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        lt_q_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        lt_q_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltxtq_exec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltxtq_rexec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _ltq_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _ltq_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _lt_q_regex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _lt_q_rregex(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _ltxtq_exec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _ltxtq_rexec(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _ltree_isparent(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        _ltree_risparent(PG_FUNCTION_ARGS);

 /* Concatenation functions */
-Datum        ltree_addltree(PG_FUNCTION_ARGS);
-Datum        ltree_addtext(PG_FUNCTION_ARGS);
-Datum        ltree_textadd(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltree_addltree(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltree_addtext(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltree_textadd(PG_FUNCTION_ARGS);

 /* Util function */
-Datum        ltree_in(PG_FUNCTION_ARGS);
+PGDLLEXPORT Datum        ltree_in(PG_FUNCTION_ARGS);

 bool        ltree_execute(ITEM *curitem, void *checkval,
                           bool calcnot, bool (*chkcond) (void *checkval, ITEM *val));
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 05c54b27de..aa153faaaf 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -259,6 +259,8 @@ SUN_STUDIO_CC = @SUN_STUDIO_CC@
 CXX = @CXX@
 CFLAGS = @CFLAGS@
 CFLAGS_SL = @CFLAGS_SL@
+CFLAGS_SL_MOD = @CFLAGS_SL_MOD@
+CXXFLAGS_SL_MOD = @CXXFLAGS_SL_MOD@
 CFLAGS_UNROLL_LOOPS = @CFLAGS_UNROLL_LOOPS@
 CFLAGS_VECTORIZE = @CFLAGS_VECTORIZE@
 CFLAGS_SSE42 = @CFLAGS_SSE42@
diff --git a/src/Makefile.shlib b/src/Makefile.shlib
index 551023c6fb..9119e9bb5d 100644
--- a/src/Makefile.shlib
+++ b/src/Makefile.shlib
@@ -253,6 +253,19 @@ ifeq ($(PORTNAME), win32)
 endif


+# If the shared library doesn't have an export file, mark all symbols not
+# explicitly exported using PGDLLEXPORT as hidden. We can't pass these flags
+# when building a library with explicit exports, as the symbols would be
+# hidden before the linker script / exported symbol list takes effect.
+#
+# XXX: This probably isn't the best location, but not clear where else
+# instead?
+ifeq ($(SHLIB_EXPORTS),)
+  LDFLAGS += $(CFLAGS_SL_MOD)
+  override CFLAGS += $(CFLAGS_SL_MOD)
+  override CXXFLAGS += $(CXXFLAGS_SL_MOD)
+endif
+

 ##
 ## BUILD
diff --git a/src/include/c.h b/src/include/c.h
index 4f16e589b3..b713123b61 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1336,14 +1336,19 @@ extern unsigned long long strtoull(const char *str, char **endptr, int base);

 /*
  * Use "extern PGDLLEXPORT ..." to declare functions that are defined in
- * loadable modules and need to be callable by the core backend.  (Usually,
- * this is not necessary because our build process automatically exports
- * such symbols, but sometimes manual marking is required.)
- * No special marking is required on most ports.
+ * loadable modules and need to be callable by the core backend or other
+ * loadable modules.
+ * If the compiler knows __attribute__((visibility("*"))), we use that,
+ * unless we already have a platform-specific definition.  Otherwise,
+ * no special marking is required.
  */
 #ifndef PGDLLEXPORT
+#ifdef HAVE_VISIBILITY_ATTRIBUTE
+#define PGDLLEXPORT __attribute__((visibility("default")))
+#else
 #define PGDLLEXPORT
 #endif
+#endif

 /*
  * The following is used as the arg list for signal handlers.  Any ports
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 6560e462d6..3c813559f3 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -413,7 +413,7 @@ typedef const Pg_finfo_record *(*PGFInfoFunction) (void);
  *    info function, since authors shouldn't need to be explicitly aware of it.
  */
 #define PG_FUNCTION_INFO_V1(funcname) \
-extern Datum funcname(PG_FUNCTION_ARGS); \
+extern PGDLLEXPORT Datum funcname(PG_FUNCTION_ARGS); \
 extern PGDLLEXPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
 const Pg_finfo_record * \
 CppConcat(pg_finfo_,funcname) (void) \
@@ -424,6 +424,16 @@ CppConcat(pg_finfo_,funcname) (void) \
 extern int no_such_variable


+/*
+ * Declare _PG_init/_PG_fini centrally. Historically each shared library had
+ * its own declaration, but now that we want to mark the symbols PGDLLEXPORT,
+ * the central declaration helps find extensions with local declarations
+ * missing PGDLLEXPORT.
+ */
+extern PGDLLEXPORT void _PG_init(void);
+extern PGDLLEXPORT void _PG_fini(void);
+
+
 /*-------------------------------------------------------------------------
  *        Support for verifying backend compatibility of loaded modules
  *
diff --git a/src/include/jit/jit.h b/src/include/jit/jit.h
index 707176d9ed..5870ec6d7b 100644
--- a/src/include/jit/jit.h
+++ b/src/include/jit/jit.h
@@ -63,7 +63,7 @@ typedef struct JitContext

 typedef struct JitProviderCallbacks JitProviderCallbacks;

-extern void _PG_jit_provider_init(JitProviderCallbacks *cb);
+extern PGDLLEXPORT void _PG_jit_provider_init(JitProviderCallbacks *cb);
 typedef void (*JitProviderInit) (JitProviderCallbacks *cb);
 typedef void (*JitProviderResetAfterErrorCB) (void);
 typedef void (*JitProviderReleaseContextCB) (JitContext *context);
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 9d9bd6b9ef..28b22bd8d6 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -701,6 +701,9 @@
 /* Define to 1 if you have the <uuid/uuid.h> header file. */
 #undef HAVE_UUID_UUID_H

+/* Define to 1 if your compiler knows the visibility("hidden") attribute. */
+#undef HAVE_VISIBILITY_ATTRIBUTE
+
 /* Define to 1 if you have the `wcstombs_l' function. */
 #undef HAVE_WCSTOMBS_L

diff --git a/src/include/replication/output_plugin.h b/src/include/replication/output_plugin.h
index 41157fda7c..490aa50825 100644
--- a/src/include/replication/output_plugin.h
+++ b/src/include/replication/output_plugin.h
@@ -35,6 +35,8 @@ typedef struct OutputPluginOptions
  */
 typedef void (*LogicalOutputPluginInit) (struct OutputPluginCallbacks *cb);

+extern PGDLLEXPORT void _PG_output_plugin_init(struct OutputPluginCallbacks *cb);
+
 /*
  * Callback that gets called in a user-defined plugin. ctx->private_data can
  * be set to some private data.
diff --git a/src/pl/plpython/plpy_elog.h b/src/pl/plpython/plpy_elog.h
index e02ef4ffe9..aeade82ce1 100644
--- a/src/pl/plpython/plpy_elog.h
+++ b/src/pl/plpython/plpy_elog.h
@@ -34,13 +34,13 @@ extern PyObject *PLy_exc_spi_error;
     } while(0)
 #endif                            /* HAVE__BUILTIN_CONSTANT_P */

-extern void PLy_elog_impl(int elevel, const char *fmt,...) pg_attribute_printf(2, 3);
+extern PGDLLEXPORT void PLy_elog_impl(int elevel, const char *fmt,...) pg_attribute_printf(2, 3);

-extern void PLy_exception_set(PyObject *exc, const char *fmt,...) pg_attribute_printf(2, 3);
+extern PGDLLEXPORT void PLy_exception_set(PyObject *exc, const char *fmt,...) pg_attribute_printf(2, 3);

-extern void PLy_exception_set_plural(PyObject *exc, const char *fmt_singular, const char *fmt_plural,
+extern PGDLLEXPORT void PLy_exception_set_plural(PyObject *exc, const char *fmt_singular, const char *fmt_plural,
                                      unsigned long n,...) pg_attribute_printf(2, 5) pg_attribute_printf(3, 5);

-extern void PLy_exception_set_with_details(PyObject *excclass, ErrorData *edata);
+extern PGDLLEXPORT void PLy_exception_set_with_details(PyObject *excclass, ErrorData *edata);

 #endif                            /* PLPY_ELOG_H */
diff --git a/src/pl/plpython/plpy_typeio.h b/src/pl/plpython/plpy_typeio.h
index d11e6ae1b8..87e3b2c464 100644
--- a/src/pl/plpython/plpy_typeio.h
+++ b/src/pl/plpython/plpy_typeio.h
@@ -147,29 +147,29 @@ struct PLyObToDatum
 };


-extern PyObject *PLy_input_convert(PLyDatumToOb *arg, Datum val);
-extern Datum PLy_output_convert(PLyObToDatum *arg, PyObject *val,
+extern PGDLLEXPORT PyObject *PLy_input_convert(PLyDatumToOb *arg, Datum val);
+extern PGDLLEXPORT Datum PLy_output_convert(PLyObToDatum *arg, PyObject *val,
                                 bool *isnull);

-extern PyObject *PLy_input_from_tuple(PLyDatumToOb *arg, HeapTuple tuple,
+extern PGDLLEXPORT PyObject *PLy_input_from_tuple(PLyDatumToOb *arg, HeapTuple tuple,
                                       TupleDesc desc, bool include_generated);

-extern void PLy_input_setup_func(PLyDatumToOb *arg, MemoryContext arg_mcxt,
+extern PGDLLEXPORT void PLy_input_setup_func(PLyDatumToOb *arg, MemoryContext arg_mcxt,
                                  Oid typeOid, int32 typmod,
                                  struct PLyProcedure *proc);
-extern void PLy_output_setup_func(PLyObToDatum *arg, MemoryContext arg_mcxt,
+extern PGDLLEXPORT void PLy_output_setup_func(PLyObToDatum *arg, MemoryContext arg_mcxt,
                                   Oid typeOid, int32 typmod,
                                   struct PLyProcedure *proc);

-extern void PLy_input_setup_tuple(PLyDatumToOb *arg, TupleDesc desc,
+extern PGDLLEXPORT void PLy_input_setup_tuple(PLyDatumToOb *arg, TupleDesc desc,
                                   struct PLyProcedure *proc);
-extern void PLy_output_setup_tuple(PLyObToDatum *arg, TupleDesc desc,
+extern PGDLLEXPORT void PLy_output_setup_tuple(PLyObToDatum *arg, TupleDesc desc,
                                    struct PLyProcedure *proc);

-extern void PLy_output_setup_record(PLyObToDatum *arg, TupleDesc desc,
+extern PGDLLEXPORT void PLy_output_setup_record(PLyObToDatum *arg, TupleDesc desc,
                                     struct PLyProcedure *proc);

 /* conversion from Python objects to C strings --- exported for transforms */
-extern char *PLyObject_AsString(PyObject *plrv);
+extern PGDLLEXPORT char *PLyObject_AsString(PyObject *plrv);

 #endif                            /* PLPY_TYPEIO_H */
diff --git a/src/pl/plpython/plpy_util.h b/src/pl/plpython/plpy_util.h
index c9ba7edc0e..6927601e0b 100644
--- a/src/pl/plpython/plpy_util.h
+++ b/src/pl/plpython/plpy_util.h
@@ -8,12 +8,12 @@

 #include "plpython.h"

-extern PyObject *PLyUnicode_Bytes(PyObject *unicode);
-extern char *PLyUnicode_AsString(PyObject *unicode);
+extern PGDLLEXPORT PyObject *PLyUnicode_Bytes(PyObject *unicode);
+extern PGDLLEXPORT char *PLyUnicode_AsString(PyObject *unicode);

 #if PY_MAJOR_VERSION >= 3
-extern PyObject *PLyUnicode_FromString(const char *s);
-extern PyObject *PLyUnicode_FromStringAndSize(const char *s, Py_ssize_t size);
+extern PGDLLEXPORT PyObject *PLyUnicode_FromString(const char *s);
+extern PGDLLEXPORT PyObject *PLyUnicode_FromStringAndSize(const char *s, Py_ssize_t size);
 #endif

 #endif                            /* PLPY_UTIL_H */
diff --git a/src/test/modules/test_shm_mq/test_shm_mq.h b/src/test/modules/test_shm_mq/test_shm_mq.h
index 0310caf50b..8f97be78d3 100644
--- a/src/test/modules/test_shm_mq/test_shm_mq.h
+++ b/src/test/modules/test_shm_mq/test_shm_mq.h
@@ -40,6 +40,6 @@ extern void test_shm_mq_setup(int64 queue_size, int32 nworkers,
                               shm_mq_handle **input);

 /* Main entrypoint for a worker. */
-extern void test_shm_mq_main(Datum) pg_attribute_noreturn();
+extern PGDLLEXPORT void test_shm_mq_main(Datum) pg_attribute_noreturn();

 #endif
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 05ced63780..a1f4dbaf3f 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -47,7 +47,7 @@ PG_MODULE_MAGIC;
 PG_FUNCTION_INFO_V1(worker_spi_launch);

 void        _PG_init(void);
-void        worker_spi_main(Datum) pg_attribute_noreturn();
+PGDLLEXPORT void worker_spi_main(Datum) pg_attribute_noreturn();

 /* GUC variables */
 static int    worker_spi_naptime = 10;
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index e47c2d648c..e076fa2a92 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -429,6 +429,7 @@ sub GenerateFiles
         HAVE_WINLDAP_H                           => undef,
         HAVE_WCSTOMBS_L                          => 1,
         HAVE_WCTYPE_H                            => 1,
+        HAVE_VISIBILITY_ATTRIBUTE                => undef,
         HAVE_WRITEV                              => undef,
         HAVE_X509_GET_SIGNATURE_NID              => 1,
         HAVE_X86_64_POPCNTQ                      => undef,

Hi,

On 2022-01-10 19:01:36 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > [ v2-0001-Use-hidden-visibility-for-shared-libraries-where-.patch ]
> 
> This seems like a good idea, but it's failing to apply right now,
> mainly because of some cleanup I did in e04a8059a.  As far as I can
> tell given the now-clarified meanings of PGDLLIMPORT/PGDLLEXPORT,
> this patch shouldn't be touching PGDLLIMPORT.

Hm. I'm not sure that's "semantically" entirely right - but for now I think
it'd have no consequences.

Without marking PGDLLIMPORT symbols as visible, the compiler / linker
compiling an extension shlib would theoretically be justified emitting a
reference that doesn't expect to go through any indirection table. Which would
lead to linker issues (about relocation sizes or undefined symbols), and could
potentially even lead to misoptimization.

However, that won't cause a problem right now, because 'extern' in a
declaration causes the reference to be to a 'default' visibility symbol (note
that the *definition* of the symbol wouldn't change).

We'd benefit from separating local cross-translation-unit (i.e. within a
shared library) from "foreign" cross-translation-unit (i.e. from extension
library into main postgres binary) symbols. But I don't really see a realistic
path to get there starting where we are today. And -Wl,-Bdynamic, -fno-plt
probably get us close enough.

It'd be a bit a different story if we could make gcc default to "local"
references to variable declarations, but not function declarations. But I
don't see an option for that.


> The attached revision works for me under gcc 8.5 and clang 13.

Thanks for doing that!


> Also, it seemed like you'd maybe been more enthusiastic than necessary
> about plastering PGDLLEXPORT on things.  I got through check-world
> cleanly without the diffs in either ltree.h or worker_spi.c (although
> I confess being confused why I didn't need the latter).

Ugh. In the case of worker_spi it's because -fvisibility=hidden isn't applied
to worker_spi.c at all. Which in turn is because Makefile.shlib isn't used at
all for MODULES= style modules just for MODULE_big= ones :(.

Once that's "fixed" it fails as expected...

I'm not sure what the best way to deal with this is. Just now I copied the
logic from Makefile.shlib to pgxs.mk (by the existing CFLAGS_SL use), but that
doesn't scale - there's other direct uses of CFLAGS_SL.

Perhaps the best way would be to add the -fvisibility=hidden to CFLAGS_SL, and
work around the explicit exports issue in Makefile.shlib by adding an explicit
-fvisibility=default? Or perhaps CFLAGS_SL


A cleaner way would probably be to bite the bullet and add explicit
PGDLLEXPORTs to all the symbols in export lists? But that's a couple hundred
functions :/.


I'll try to crawl into the dusty path of a few months ago, and figure out why
I thought the ltree additions are needed...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-01-10 21:11:07 -0600, Justin Pryzby wrote:
>> Without the patch, it fails under windows like:

> Ah, yea. It's about matching the declarations in ltree.h with the
> PG_FUNCTION_INFO_V1() one.

> What is bugging me is that I am fairly sure that my local compilers at some
> point complained about such mismatches on linux as well. But I can't reproduce
> that right now :/

Ah, I wondered about that.  I'd sort of expected warnings from
mismatched declarations, but I didn't see any on Linux or macOS.

            regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> What is bugging me is that I am fairly sure that my local compilers at some
> point complained about such mismatches on linux as well. But I can't reproduce
> that right now :/

> Now I wonder if I just saw it when cross compiling locally...

I still don't understand what are the conditions for MSVC to complain.
The rule is evidently not that every extern must agree with the function
definition, because for example you added

+extern PGDLLEXPORT void _PG_init(void);

in fmgr.h, but you didn't change any of the existing extern declarations
or definitions for _PG_init functions, and yet everything seems to work.

I had concluded that gcc/clang follow the rule "use an attribute if it
appears on at least one extern for the function", and this seems like
evidence that it works like that in MSVC too.

            regards, tom lane

On 11.01.22 03:53, Andres Freund wrote:
> Ugh. In the case of worker_spi it's because -fvisibility=hidden isn't applied
> to worker_spi.c at all. Which in turn is because Makefile.shlib isn't used at
> all for MODULES= style modules just for MODULE_big= ones :(.
> 
> Once that's "fixed" it fails as expected...
> 
> I'm not sure what the best way to deal with this is. Just now I copied the
> logic from Makefile.shlib to pgxs.mk (by the existing CFLAGS_SL use), but that
> doesn't scale - there's other direct uses of CFLAGS_SL.
> 
> Perhaps the best way would be to add the -fvisibility=hidden to CFLAGS_SL, and
> work around the explicit exports issue in Makefile.shlib by adding an explicit
> -fvisibility=default? Or perhaps CFLAGS_SL

The easiest solution would be to change worker_spi's Makefile to use 
MODULE_big.

There are already many cases where MODULE_big is used instead of MODULES 
for seemingly random reasons, so I wouldn't worry too much about it here 
if it helps you move forward.

Hi,

On 2022-01-11 15:54:19 -0500, Tom Lane wrote:
> I still don't understand what are the conditions for MSVC to complain.
> The rule is evidently not that every extern must agree with the function
> definition, because for example you added
> 
> +extern PGDLLEXPORT void _PG_init(void);
> 
> in fmgr.h, but you didn't change any of the existing extern declarations
> or definitions for _PG_init functions, and yet everything seems to work.

I think I figured that part out now:
https://godbolt.org/z/qYqo95fYs

It works as long as the *first* declaration has the declspec, later ones don't
need it. If the first one does *not* have the declspec but later ones don't,
you get "error C2375: 'msvc_fail': redefinition; different linkage".

That makes some sort of sense.


> I had concluded that gcc/clang follow the rule "use an attribute if it
> appears on at least one extern for the function", and this seems like
> evidence that it works like that in MSVC too.

So it's not quite the same as with gcc / clang...

Greetings,

Andres Freund

Hi,

On 2022-03-24 16:13:31 +0100, Peter Eisentraut wrote:
> On 11.01.22 03:53, Andres Freund wrote:
> > Ugh. In the case of worker_spi it's because -fvisibility=hidden isn't applied
> > to worker_spi.c at all. Which in turn is because Makefile.shlib isn't used at
> > all for MODULES= style modules just for MODULE_big= ones :(.
> >
> > Once that's "fixed" it fails as expected...
> >
> > I'm not sure what the best way to deal with this is. Just now I copied the
> > logic from Makefile.shlib to pgxs.mk (by the existing CFLAGS_SL use), but that
> > doesn't scale - there's other direct uses of CFLAGS_SL.
> >
> > Perhaps the best way would be to add the -fvisibility=hidden to CFLAGS_SL, and
> > work around the explicit exports issue in Makefile.shlib by adding an explicit
> > -fvisibility=default? Or perhaps CFLAGS_SL
>
> The easiest solution would be to change worker_spi's Makefile to use
> MODULE_big.
>
> There are already many cases where MODULE_big is used instead of MODULES for
> seemingly random reasons, so I wouldn't worry too much about it here if it
> helps you move forward.

If it were just worker_spi that might be tenable, but there's other extension
modules - even if they don't fail to fail right now, we shouldn't change the
symbol export rules based on MODULES vs MODULE_big.

I think the idea that I rejected above, namely adding CFLAGS_SL_MOD in pgxs.mk
is the best approach. There aren't actually that many uses of CFLAGS_SL
otherwise - not quite sure anymore why I thought that to be bad.

Is there any reason an extension module would need to directly link against
pgport/pgcommon? I don't think so, right?


What I'm not sure about is what to do about pg_config - we can't just add
-fvisibility=hidden to pg_config --cflags_sl. We could add --cflags_sl_mod -
but I'm not sure it's worth it?

In the attached version, based on Tom's version upthread, I changed the
following:
- moved adding central declarations for _PG_init etc to a separate patch, now
  also removes the now duplicated decls
- split addition of PGDLLEXPORT to symbols into a separate patch
- added a central PGDLLEXPORT'ed prototype for_PG_archive_module_init
- Removed .DEF file generation for liraries in src/tools/msvc, only the
  backend now generates a DEF file for all symbols ([1])
- added the flags to pgxs.mk MODULES
- pgindented changed files

I added CFLAGS_SL_MOD to LDFLAGS_SL, but right now that's not really
necessary, it seems all places using LDFLAGS_SL also use CFLAGS. Which is a
bit weird, but ...

There are a few symbols in plpython that don't need to be exported right now
but are. But it seems better to export a few too many there, as the
alternative is breaking out-of-core transforms. Most of the symbols are used
by the various transform extensions.

Greetings,

Andres Freund


[1] It's not too hard to move away from that, and I suspect it'd be worth
    it. I did prototype it. But that's a relatively large change that imo
    better is discussed separately.

Andres Freund <andres@anarazel.de> writes:
> On 2022-03-24 16:13:31 +0100, Peter Eisentraut wrote:
>> The easiest solution would be to change worker_spi's Makefile to use
>> MODULE_big.

> If it were just worker_spi that might be tenable, but there's other extension
> modules - even if they don't fail to fail right now, we shouldn't change the
> symbol export rules based on MODULES vs MODULE_big.

Agreed, that doesn't seem nice.

> Is there any reason an extension module would need to directly link against
> pgport/pgcommon? I don't think so, right?

Shouldn't --- we want it to use the backend's own copy.  (Hmm ... but
what if there's some file in one of those libraries that is not used
by the core backend, but is used by the extension?)  In any case, why
would that matter for this patch?  If an extension does link in such
a file, for sure we don't want that exposed outside the extension.

> What I'm not sure about is what to do about pg_config - we can't just add
> -fvisibility=hidden to pg_config --cflags_sl. We could add --cflags_sl_mod -
> but I'm not sure it's worth it?

Don't have a strong opinion here.  But if we're forcing
-fvisibility=hidden on modules built with pgxs, I'm not sure why
we can't do so for modules relying on pg_config.

> There are a few symbols in plpython that don't need to be exported right now
> but are. But it seems better to export a few too many there, as the
> alternative is breaking out-of-core transforms. Most of the symbols are used
> by the various transform extensions.

Concur.

I wanted to test this on a compiler lacking -fvisibility, and was somewhat
amused to discover that I haven't got one --- even prairiedog's ancient
gcc 4.0.1 knows it.  Maybe some of the vendor-specific compilers in the
buildfarm will be able to verify that aspect for us.

I have a couple of very minor nitpicks:

1. The comment for the shared _PG_init/_PG_fini declarations could be
worded better, perhaps like

 * Declare _PG_init/_PG_fini centrally. Historically each shared library had
 * its own declaration; but now that we want to mark these PGDLLEXPORT,
 * using central declarations avoids each extension having to add that.
 * Any existing declarations in extensions will continue to work if fmgr.h
 * is included before them, otherwise compilation for Windows will fail.

2. I'm not really thrilled with the symbol names C[XX]FLAGS_SL_MOD;
it's not apparent what the MOD means, nor is that documented anywhere.
Maybe C[XX]FLAGS_SL_VISIBILITY?  In any case a comment explaining
when these are meant to be used might help extension developers.
(Although now that I look, there seems noplace documenting what
CFLAG_SL is either :-(.)

Beyond that, I think this is committable.  We're not likely to learn much
more about any potential issues except by exposing it to the buildfarm
and developer usage.

            regards, tom lane

Hi,

Thanks for the quick review.


On 2022-07-17 14:54:48 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Is there any reason an extension module would need to directly link against
> > pgport/pgcommon? I don't think so, right?
> 
> Shouldn't --- we want it to use the backend's own copy.  (Hmm ... but
> what if there's some file in one of those libraries that is not used
> by the core backend, but is used by the extension?)  In any case, why
> would that matter for this patch?  If an extension does link in such
> a file, for sure we don't want that exposed outside the extension.

I was wondering whether there is a reason {pgport,pgcommon}_srv should ever be
built with -fno-visibility. Right now there's no reason to do so, but if an
extension .so would directly link to them, they'd have to built with that. But
until / unless we add visibility information to all backend functions
declarations, we can't do that for the versions the backend uses.


> > What I'm not sure about is what to do about pg_config - we can't just add
> > -fvisibility=hidden to pg_config --cflags_sl. We could add --cflags_sl_mod -
> > but I'm not sure it's worth it?
> 
> Don't have a strong opinion here.  But if we're forcing
> -fvisibility=hidden on modules built with pgxs, I'm not sure why
> we can't do so for modules relying on pg_config.

If an extension were to use pg_config to build a "proper" shared library
(rather than our more plugin like extension libraries), they might not want
the -fvisibility=hidden, e.g. if they use a mechanism like our exports.txt.

That's also the reason -fvisibility=hidden isn't added to libpq and the ecpg
libs - their symbol visility is done via exports.txt. Depending on the
platform that'd not work if symbols were already hidden via
-fvisibility=hidden (unless explicitly exported via PGDLLEXPORT, of
course).

It might or might not be worth switching to PGDLLEXPORT for those in the
future, but that's imo a separate discussion.


> I wanted to test this on a compiler lacking -fvisibility, and was somewhat
> amused to discover that I haven't got one --- even prairiedog's ancient
> gcc 4.0.1 knows it.  Maybe some of the vendor-specific compilers in the
> buildfarm will be able to verify that aspect for us.

Heh. Even AIX's compiler knows it, and I think sun studio as well.  MSVC
obviously doesn't, but we have declspec support to deal with that. It's
possible that HP-UX's acc doesn't, but that's gone now...  It's possible that
there's ABIs targeted by gcc that don't have symbol visibility support - but I
can't think of any we support of the top of my head.

IOW, we could likely rely on symbol visibility support, if there's advantage
in that.


> I have a couple of very minor nitpicks:
> 
> 1. The comment for the shared _PG_init/_PG_fini declarations could be
> worded better, perhaps like
> 
>  * Declare _PG_init/_PG_fini centrally. Historically each shared library had
>  * its own declaration; but now that we want to mark these PGDLLEXPORT,
>  * using central declarations avoids each extension having to add that.
>  * Any existing declarations in extensions will continue to work if fmgr.h
>  * is included before them, otherwise compilation for Windows will fail.

WFM.


> 2. I'm not really thrilled with the symbol names C[XX]FLAGS_SL_MOD;
> it's not apparent what the MOD means, nor is that documented anywhere.

It's for module, which I got from pgxs' MODULES/MODULE_big (and incidentally
that's also what meson calls it). Definitely should be explained, and perhaps
be expanded to MODULE.


> Maybe C[XX]FLAGS_SL_VISIBILITY?  In any case a comment explaining
> when these are meant to be used might help extension developers.
> (Although now that I look, there seems noplace documenting what
> CFLAG_SL is either :-(.)

I was thinking that it might be nicer to bundle the flags that should be
applied to extension libraries together. I don't think we have others right
now, but I think that might change (e.g. -fno-plt -fno-semantic-interposition,
-Wl,-Bdynamic would make sense).

I'm not wedded to this, but I think it makes some sense?


> Beyond that, I think this is committable.  We're not likely to learn much
> more about any potential issues except by exposing it to the buildfarm
> and developer usage.

Cool. I'll work on committing the first two then and see what comes out of the
CFLAGS_SL_* discussion.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> That's also the reason -fvisibility=hidden isn't added to libpq and the ecpg
> libs - their symbol visility is done via exports.txt. Depending on the
> platform that'd not work if symbols were already hidden via
> -fvisibility=hidden (unless explicitly exported via PGDLLEXPORT, of
> course).

Got it.  Yeah, let's not break those cases.  (I don't think we dare put
PGDLLEXPORT into libpq-fe.h, for example, so it might be hard to solve
it any other way anyhow.)

>> 2. I'm not really thrilled with the symbol names C[XX]FLAGS_SL_MOD;
>> it's not apparent what the MOD means, nor is that documented anywhere.

> It's for module, which I got from pgxs' MODULES/MODULE_big (and incidentally
> that's also what meson calls it). Definitely should be explained, and perhaps
> be expanded to MODULE.

I guessed as much, but +1 for spelling it out as MODULE.

            regards, tom lane

Hi,

On 2022-07-17 14:54:48 -0400, Tom Lane wrote:
> Beyond that, I think this is committable.  We're not likely to learn much
> more about any potential issues except by exposing it to the buildfarm
> and developer usage.

Leaving egg on my face aside, seems to work well so far.

It looks like we might be missing out on benefiting from this on more
platforms - the test right now is in the gcc specific section of configure.ac,
but it looks like at least Intel's, sun's and AIX's compilers might all
support -fvisibility with the same syntax.

Given that that's just about all compilers we support using configure, perhaps
we should just move that out of the compiler specific section? Doesn't look
like there's much precedent for that so far...

Greetings,

Andres Freund

Hi,

On 2022-07-18 00:05:16 -0700, Andres Freund wrote:
> It looks like we might be missing out on benefiting from this on more
> platforms - the test right now is in the gcc specific section of configure.ac,
> but it looks like at least Intel's, sun's and AIX's compilers might all
> support -fvisibility with the same syntax.
> 
> Given that that's just about all compilers we support using configure, perhaps
> we should just move that out of the compiler specific section? Doesn't look
> like there's much precedent for that so far...

Here's a potential patch along those lines.


I wonder if we also should move the -fno-strict-aliasing, -fwrapv tests
out. But that'd be something for later.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-07-18 00:05:16 -0700, Andres Freund wrote:
>> Given that that's just about all compilers we support using configure, perhaps
>> we should just move that out of the compiler specific section? Doesn't look
>> like there's much precedent for that so far...

> Here's a potential patch along those lines.

Now that the dust from the main patch is pretty well settled, +1
for trying that.

> I wonder if we also should move the -fno-strict-aliasing, -fwrapv tests
> out. But that'd be something for later.

Those seem less likely to be portable to non-gcc-alike compilers.
On the other hand, maybe it'd be interesting to just remove the
conditionality temporarily and try ALL the switches on all compilers,
just to see what we can learn from the buildfarm.

            regards, tom lane

Hi,

On 2022-07-27 14:02:28 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-07-18 00:05:16 -0700, Andres Freund wrote:
> >> Given that that's just about all compilers we support using configure, perhaps
> >> we should just move that out of the compiler specific section? Doesn't look
> >> like there's much precedent for that so far...
> 
> > Here's a potential patch along those lines.
> 
> Now that the dust from the main patch is pretty well settled, +1
> for trying that.

Here's an updated patch for this (also shared recently on another thread). I
was wrong earlier saying that -fvisibility works for xlc - it is accepted, but
just as some sort of input file. We do need -qvisibility.

I tested it on aix and solaris, both with gcc and their proprietary compilers
and confirmed that it indeed does reduce the number of exposed symbols.

I also attached the aix patch to not use mkldexport for extension anymore, as
I tested the patches in that order. I plan to push the aix one as soon as as
hoverfly and sungazer ran once after e5484554ba9.


> > I wonder if we also should move the -fno-strict-aliasing, -fwrapv tests
> > out. But that'd be something for later.
> 
> Those seem less likely to be portable to non-gcc-alike compilers.
> On the other hand, maybe it'd be interesting to just remove the
> conditionality temporarily and try ALL the switches on all compilers,
> just to see what we can learn from the buildfarm.

Unfortunately, given the discovery of xlc accepting -f... silently that's
probably not feasible. We'd have to make it at least conditional on not being
xlc. Everything else we support does seem to be ok with -fxxx flags.

Greetings,

Andres Freund

Hi,

On 2022-09-01 14:19:35 -0700, Andres Freund wrote:
> Here's an updated patch for this (also shared recently on another thread).

I've since then committed this.

I was reminded of this when thinking about
https://postgr.es/m/1595488.1665869988%40sss.pgh.pa.us

Thinking about this made me realize that we currently don't expose
-fvisibility=hidden via pg_config. It's added to extensions built via PGXS,
via Makefile.global, but that doesn't help extensions that don't want to use
PGXS.

I guess we should report the CFLAGS_SL_MODULE etc via pg_config?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> I guess we should report the CFLAGS_SL_MODULE etc via pg_config?

Dunno ... where do you stop?  I'm not really convinced that extensions
that don't want to use PGXS have any claim on our buildsystem to provide
platform-specific configuration details for them.

            regards, tom lane

Hi,

On 2022-10-15 22:50:07 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I guess we should report the CFLAGS_SL_MODULE etc via pg_config?
>
> Dunno ... where do you stop?  I'm not really convinced that extensions
> that don't want to use PGXS have any claim on our buildsystem to provide
> platform-specific configuration details for them.

That works for me. I just didn't want to decide on that "by fiat" after I
noticed it...

Greetings,

Andres Freund