Hi,
Thanks for the quick review.
On 2022-07-17 14:54:48 -0400, Tom Lane wrote:
> Andres Freund <and...@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
