Re: Use -fvisibility=hidden for shared libraries

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