Re: Use -fvisibility=hidden for shared libraries

Andres Freund <andres(at)anarazel(dot)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