From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        Laurenz Albe <laurenz.albe@cybertec.at>, pgsql-hackers@postgresql.org
Subject: Re: Use -fvisibility=hidden for shared libraries
In-reply-to: <20220716201750.qq6xwegkgp7xqi5w@awork3.anarazel.de>
References: <20211101020311.av6hphdl6xbjbuif@alap3.anarazel.de>
 <b8103d16bf26cff7a393d1de690474dbd9afaa19.camel@cybertec.at>
 <20211101054700.z3jzmc5lnxlad4y7@alap3.anarazel.de>
 <2499085.1641859296@sss.pgh.pa.us>
 <20220111025328.iq5g6uck53j5qtin@alap3.anarazel.de>
 <5b8d48a4-2fee-6764-7eda-a076f544ea33@enterprisedb.com>
 <20220716201750.qq6xwegkgp7xqi5w@awork3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Sat, 16 Jul 2022 13:17:50 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <180137.1658084088.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 17 Jul 2022 14:54:48 -0400
Message-ID: <180138.1658084088@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/180138.1658084088%40sss.pgh.pa.us>
Precedence: bulk

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 exte=
nsion
> 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 agai=
nst
> 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 ad=
d
> -fvisibility=3Dhidden 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=3Dhidden 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 h=
ad
 * 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