Re: Use -fvisibility=hidden for shared libraries

Hi,

On 2022-01-10 19:01:36 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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