Horton

Thread

Commits

  1. Mark all symbols exported from extension libraries PGDLLEXPORT.

  2. Default to hidden visibility for extension libraries where possible

  3. Remove now superfluous declarations of dlsym()ed symbols.

  4. Add central declarations for dlsym()ed symbols

  1. Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2021-11-01T02:03:11Z

    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

  2. Re: Use -fvisibility=hidden for shared libraries

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-11-01T04:55:40Z

    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

  3. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2021-11-01T05:47:00Z

    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

  4. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-11T00:01:36Z

    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

  5. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-01-11T02:53:28Z

    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

  6. Re: Use -fvisibility=hidden for shared libraries

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-11T03:11:07Z

    On Mon, Jan 10, 2022 at 07:01:36PM -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.  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.

Without the patch, it fails under windows like:

https://cirrus-ci.com/task/6171477065072640
[01:44:45.399] c:\cirrus\contrib\ltree\ltree.h(193): message : see declaration of '_ltree_isparent' [c:\cirrus\ltree.vcxproj]
[01:44:45.399] c:\cirrus\contrib\ltree\_ltree_op.c(16,1): error C2375: '_ltree_risparent': redefinition; different linkage [c:\cirrus\ltree.vcxproj]

-- 
Justin

  7. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-01-11T03:31:33Z

    Hi,

On 2022-01-10 21:11:07 -0600, Justin Pryzby wrote:
> On Mon, Jan 10, 2022 at 07:01:36PM -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.  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.
> 
> Without the patch, it fails under windows like:
> 
> https://cirrus-ci.com/task/6171477065072640
> [01:44:45.399] c:\cirrus\contrib\ltree\ltree.h(193): message : see declaration of '_ltree_isparent' [c:\cirrus\ltree.vcxproj]
> [01:44:45.399] c:\cirrus\contrib\ltree\_ltree_op.c(16,1): error C2375: '_ltree_risparent': redefinition; different linkage [c:\cirrus\ltree.vcxproj]

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 :/

Greetings,

Andres Freund

  8. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-11T04:44:06Z

    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

  9. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-01-11T05:42:52Z

    Hi,

On 2022-01-10 23:44:06 -0500, Tom Lane wrote:
> 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 :/

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


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

There are warnings when the declarations "explicitly" mismatch, e.g. one with
__attribute__((visibility("hidden"))) and one with
__attribute__((visibility("default"))). But -fvisibility=hidden isn't
sufficient.

Which makes sense to some degree, because of -fvisibility not applying to
extern declarations... But it's still annoying.

Greetings,

Andres Freund

  10. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-11T20:54:19Z

    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

  11. Re: Use -fvisibility=hidden for shared libraries

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-24T15:13:31Z

    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.

  12. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-07-16T00:13:19Z

    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

  13. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-07-16T20:17:50Z

    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.

  14. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-17T18:54:48Z

    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

  15. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-07-17T20:00:41Z

    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

  16. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-17T20:06:28Z

    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

  17. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-07-18T07:05:16Z

    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

  18. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-07-18T22:09:38Z

    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

  19. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-27T18:02:28Z

    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

  20. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-09-01T21:19:35Z

    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

  21. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-10-16T02:08:08Z

    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

  22. Re: Use -fvisibility=hidden for shared libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-16T02:50:07Z

    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

  23. Re: Use -fvisibility=hidden for shared libraries

    Andres Freund <andres@anarazel.de> — 2022-10-16T05:25:22Z

    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