Horton

Thread

Commits

  1. mingw: Define PGDLLEXPORT as __declspec (dllexport) as done for msvc

  2. Fix PL/Perl build on Cygwin

  3. Remove further unwanted linker flags from perl_embed_ldflags

  1. Strip -mmacosx-version-min options from plperl build

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-18T06:57:51Z

    When building on macOS against a Homebrew-provided Perl installation, I 
get these warnings during the build:

ld: warning: object file (SPI.o) was built for newer macOS version 
(12.4) than being linked (11.3)
ld: warning: object file (plperl.o) was built for newer macOS version 
(12.4) than being linked (11.3)
...

This is because the link command uses the option 
-mmacosx-version-min=11.3, which comes in from perl_embed_ldflags (perl 
-MExtUtils::Embed -e ldopts), but the compile commands don't use that 
option, which creates a situation that ld considers inconsistent.

I think an appropriate fix is to strip out the undesired option from 
perl_embed_ldflags.  We already do that for other options.  Proposed 
patch attached.

  2. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-18T13:53:48Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> This is because the link command uses the option 
> -mmacosx-version-min=11.3, which comes in from perl_embed_ldflags (perl 
> -MExtUtils::Embed -e ldopts), but the compile commands don't use that 
> option, which creates a situation that ld considers inconsistent.

> I think an appropriate fix is to strip out the undesired option from 
> perl_embed_ldflags.  We already do that for other options.  Proposed 
> patch attached.

Agreed on rejecting -mmacosx-version-min, but I wonder if we should
think about adopting a whitelist-instead-of-blacklist approach to
adopting stuff from perl_embed_ldflags.  ISTR that in pltcl we already
use the approach of accepting only -L and -l, and perhaps similar
strictness would serve us well here.

As an example, on a not-too-new MacPorts install, I see

$ /opt/local/bin/perl -MExtUtils::Embed -e ldopts
    -L/opt/local/lib -Wl,-headerpad_max_install_names   -fstack-protector-strong  -L/opt/local/lib/perl5/5.28/darwin-thread-multi-2level/CORE -lperl

I can't see any really good reason why we should allow perl
to be injecting that sort of -f option into the plperl build,
and I'm pretty dubious about the -headerpad_max_install_names
bit too.

I think also that this would allow us to drop the weird dance of
trying to subtract ccdlflags.

			regards, tom lane

  3. Re: Strip -mmacosx-version-min options from plperl build

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-19T07:12:06Z

    On 18.08.22 15:53, Tom Lane wrote:
> Agreed on rejecting -mmacosx-version-min, but I wonder if we should
> think about adopting a whitelist-instead-of-blacklist approach to
> adopting stuff from perl_embed_ldflags.  ISTR that in pltcl we already
> use the approach of accepting only -L and -l, and perhaps similar
> strictness would serve us well here.
> 
> As an example, on a not-too-new MacPorts install, I see
> 
> $ /opt/local/bin/perl -MExtUtils::Embed -e ldopts
>      -L/opt/local/lib -Wl,-headerpad_max_install_names   -fstack-protector-strong  -L/opt/local/lib/perl5/5.28/darwin-thread-multi-2level/CORE -lperl
> 
> I can't see any really good reason why we should allow perl
> to be injecting that sort of -f option into the plperl build,
> and I'm pretty dubious about the -headerpad_max_install_names
> bit too.
> 
> I think also that this would allow us to drop the weird dance of
> trying to subtract ccdlflags.

After analyzing the source code of ExtUtils::Embed's ldopts, I think we 
can also do this by subtracting $Config{ldflags}, since

my $linkage = "$ccdlflags $ldflags @archives $ld_or_bs";

and we really just want the $ld_or_bs part. (@archives should be empty 
for our uses.)

This would get rid of -mmacosx-version-min and -arch and all the things 
you showed, including -L/opt/local/lib, which is probably there so that 
the build of Perl itself could look there for things, but we don't need it.

  4. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-19T14:00:35Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> After analyzing the source code of ExtUtils::Embed's ldopts, I think we 
> can also do this by subtracting $Config{ldflags}, since
> my $linkage = "$ccdlflags $ldflags @archives $ld_or_bs";
> and we really just want the $ld_or_bs part. (@archives should be empty 
> for our uses.)

+1, this looks like a nice clean solution.  I see that it gets rid
of stuff we don't really want on RHEL8 as well as various generations
of macOS.

> This would get rid of -mmacosx-version-min and -arch and all the things 
> you showed, including -L/opt/local/lib, which is probably there so that 
> the build of Perl itself could look there for things, but we don't need it.

It is a little weird that they are inserting -L/opt/local/lib or
-L/usr/local/lib on so many different platforms.  But I concur
that if we need that, we likely should be inserting it ourselves
rather than absorbing it from their $ldflags.

BTW, I think the -arch business is dead code anyway now that we
desupported PPC-era macOS; I do not see any such switches from
modern macOS' perl.  So not having a special case for that is an
additional win.

Patch LGTM; I noted only a trivial typo in the commit message:

-like we already do with $Config{ccdlflags}.  Those flags the choices
+like we already do with $Config{ccdlflags}.  Those flags are the choices

			regards, tom lane

  5. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-20T20:44:01Z

    Hi,

On 2022-08-19 10:00:35 -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> > After analyzing the source code of ExtUtils::Embed's ldopts, I think we
> > can also do this by subtracting $Config{ldflags}, since
> > my $linkage = "$ccdlflags $ldflags @archives $ld_or_bs";
> > and we really just want the $ld_or_bs part. (@archives should be empty
> > for our uses.)
>
> +1, this looks like a nice clean solution.  I see that it gets rid
> of stuff we don't really want on RHEL8 as well as various generations
> of macOS.

Looks like it'd also get rid of the bogus
-bE:/usr/opt/perl5/lib64/5.28.1/aix-thread-multi-64all/CORE/perl.exp we're
we're picking up on AIX (we had a thread about filtering that out, but I've only
done so inside the meson patch, round tuits).

So +1 from that front.


Maybe a daft question: Why do want any of the -l flags other than -lperl? With
the patch configure spits out the following on my debian system:

checking for CFLAGS to compile embedded Perl... -DDEBIAN
checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt

those libraries were likely relevant to build libperl, but don't look relevant
for linking to it dynamically. Statically would be a different story, but we
already insist on a shared build.

Greetings,

Andres Freund

  6. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-20T20:53:31Z

    Andres Freund <andres@anarazel.de> writes:
> Maybe a daft question: Why do want any of the -l flags other than -lperl? With
> the patch configure spits out the following on my debian system:

> checking for CFLAGS to compile embedded Perl... -DDEBIAN
> checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt

> those libraries were likely relevant to build libperl, but don't look relevant
> for linking to it dynamically.

I'm certain that there are/were platforms that insist on those libraries
being mentioned anyway.  Maybe they are all obsolete now?

			regards, tom lane

  7. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-20T21:44:47Z

    Hi,

FWIW, looks like Peter's patch unbreaks building plperl on AIX using gcc and
system perl. Before we picked up a bunch of xlc specific flags that prevented
that.

before:
checking for flags to link embedded Perl...  -brtl -bdynamic -b64  -L/usr/opt/perl5/lib64/5.28.1/aix-thread-multi-64all/CORE -lperl -lpthread -lbind -lnsl -ldl -lld -lm -lcrypt -lpthreads -lc
now:
checking for flags to link embedded Perl...   -L/usr/opt/perl5/lib64/5.28.1/aix-thread-multi-64all/CORE -lperl -lpthread -lbind -lnsl -ldl -lld -lm -lcrypt -lpthreads -lc


On 2022-08-20 16:53:31 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Maybe a daft question: Why do want any of the -l flags other than -lperl? With
> > the patch configure spits out the following on my debian system:
>
> > checking for CFLAGS to compile embedded Perl... -DDEBIAN
> > checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt
>
> > those libraries were likely relevant to build libperl, but don't look relevant
> > for linking to it dynamically.
>
> I'm certain that there are/were platforms that insist on those libraries
> being mentioned anyway.  Maybe they are all obsolete now?

I don't think any of the supported platforms require it for stuff used inside
the shared library (and we'd be in trouble if so, check e.g. libpq.pc). But of
course that's different if there's inline function / macros getting pulled in.

Which turns out to be an issue on AIX. All the -l flags added by perl can be
removed for xlc, but for gcc, -lpthreads (or -pthread) it is required.

Tried it on Solaris (32 bit, not sure if there's a 64bit perl available),
works.

Greetings,

Andres Freund

  8. Re: Strip -mmacosx-version-min options from plperl build

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-22T14:31:53Z

    On 20.08.22 22:44, Andres Freund wrote:
> Maybe a daft question: Why do want any of the -l flags other than -lperl? With
> the patch configure spits out the following on my debian system:
> 
> checking for CFLAGS to compile embedded Perl... -DDEBIAN
> checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt
> 
> those libraries were likely relevant to build libperl, but don't look relevant
> for linking to it dynamically. Statically would be a different story, but we
> already insist on a shared build.

Looking inside the ExtUtils::Embed source code, I wonder if there are 
some installations that have things like -lperl538 or something like 
that that it wants to deal with.

  9. Re: Strip -mmacosx-version-min options from plperl build

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-22T14:32:36Z

    On 20.08.22 23:44, Andres Freund wrote:
> On 2022-08-20 16:53:31 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> Maybe a daft question: Why do want any of the -l flags other than -lperl? With
>>> the patch configure spits out the following on my debian system:
>>
>>> checking for CFLAGS to compile embedded Perl... -DDEBIAN
>>> checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt
>>
>>> those libraries were likely relevant to build libperl, but don't look relevant
>>> for linking to it dynamically.
>>
>> I'm certain that there are/were platforms that insist on those libraries
>> being mentioned anyway.  Maybe they are all obsolete now?
> 
> I don't think any of the supported platforms require it for stuff used inside
> the shared library (and we'd be in trouble if so, check e.g. libpq.pc). But of
> course that's different if there's inline function / macros getting pulled in.
> 
> Which turns out to be an issue on AIX. All the -l flags added by perl can be
> removed for xlc, but for gcc, -lpthreads (or -pthread) it is required.
> 
> Tried it on Solaris (32 bit, not sure if there's a 64bit perl available),
> works.

Does that mean my proposed patch (v2) is adequate for these platforms, 
or does it need further analysis?

  10. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-22T15:37:40Z

    Hi,

On 2022-08-22 16:32:36 +0200, Peter Eisentraut wrote:
> On 20.08.22 23:44, Andres Freund wrote:
> > On 2022-08-20 16:53:31 -0400, Tom Lane wrote:
> > > Andres Freund <andres@anarazel.de> writes:
> > > > Maybe a daft question: Why do want any of the -l flags other than -lperl? With
> > > > the patch configure spits out the following on my debian system:
> > > 
> > > > checking for CFLAGS to compile embedded Perl... -DDEBIAN
> > > > checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt
> > > 
> > > > those libraries were likely relevant to build libperl, but don't look relevant
> > > > for linking to it dynamically.
> > > 
> > > I'm certain that there are/were platforms that insist on those libraries
> > > being mentioned anyway.  Maybe they are all obsolete now?
> > 
> > I don't think any of the supported platforms require it for stuff used inside
> > the shared library (and we'd be in trouble if so, check e.g. libpq.pc). But of
> > course that's different if there's inline function / macros getting pulled in.
> > 
> > Which turns out to be an issue on AIX. All the -l flags added by perl can be
> > removed for xlc, but for gcc, -lpthreads (or -pthread) it is required.
> > 
> > Tried it on Solaris (32 bit, not sure if there's a 64bit perl available),
> > works.
> 
> Does that mean my proposed patch (v2) is adequate for these platforms, or
> does it need further analysis?

I think it's a clear improvement over the status quo. Unnecessary -l's are
pretty harmless compared to random other flags.

Greetings,

Andres Freund

  11. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-22T15:41:34Z

    Hi,

On 2022-08-22 16:31:53 +0200, Peter Eisentraut wrote:
> On 20.08.22 22:44, Andres Freund wrote:
> > Maybe a daft question: Why do want any of the -l flags other than -lperl? With
> > the patch configure spits out the following on my debian system:
> > 
> > checking for CFLAGS to compile embedded Perl... -DDEBIAN
> > checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt
> > 
> > those libraries were likely relevant to build libperl, but don't look relevant
> > for linking to it dynamically. Statically would be a different story, but we
> > already insist on a shared build.
> 
> Looking inside the ExtUtils::Embed source code, I wonder if there are some
> installations that have things like -lperl538 or something like that that it
> wants to deal with.

There definitely are - I wasn't trying to suggest we'd add -lperl ourselves,
just that we'd try to only add -lperl* based on some Config variable.

We have plenty fragile windows specific logic that would be nice to get rid
of. But there's more exciting things to wrangle, I have to admit.

Greetings,

Andres Freund

  12. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-24T02:11:50Z

    Hi,

On 2022-08-22 08:37:40 -0700, Andres Freund wrote:
> On 2022-08-22 16:32:36 +0200, Peter Eisentraut wrote:
> > On 20.08.22 23:44, Andres Freund wrote:
> > > On 2022-08-20 16:53:31 -0400, Tom Lane wrote:
> > > > Andres Freund <andres@anarazel.de> writes:
> > > > > Maybe a daft question: Why do want any of the -l flags other than -lperl? With
> > > > > the patch configure spits out the following on my debian system:
> > > > 
> > > > > checking for CFLAGS to compile embedded Perl... -DDEBIAN
> > > > > checking for flags to link embedded Perl...   -L/usr/lib/x86_64-linux-gnu/perl/5.34/CORE -lperl -ldl -lm -lpthread -lc -lcrypt
> > > > 
> > > > > those libraries were likely relevant to build libperl, but don't look relevant
> > > > > for linking to it dynamically.
> > > > 
> > > > I'm certain that there are/were platforms that insist on those libraries
> > > > being mentioned anyway.  Maybe they are all obsolete now?
> > > 
> > > I don't think any of the supported platforms require it for stuff used inside
> > > the shared library (and we'd be in trouble if so, check e.g. libpq.pc). But of
> > > course that's different if there's inline function / macros getting pulled in.
> > > 
> > > Which turns out to be an issue on AIX. All the -l flags added by perl can be
> > > removed for xlc, but for gcc, -lpthreads (or -pthread) it is required.
> > > 
> > > Tried it on Solaris (32 bit, not sure if there's a 64bit perl available),
> > > works.
> > 
> > Does that mean my proposed patch (v2) is adequate for these platforms, or
> > does it need further analysis?
> 
> I think it's a clear improvement over the status quo. Unnecessary -l's are
> pretty harmless compared to random other flags.

FWIW, while trying to mirror the same logic in meson I learned that the new
logic removes the rpath setting from the parameters on at least netbsd and
suse. We'll add them back - unless --disable-rpath is used. I think some
distributions build with --disable-rpath. Not sure if worth worrying about.

Greetings,

Andres Freund

  13. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-24T02:49:25Z

    Andres Freund <andres@anarazel.de> writes:
> FWIW, while trying to mirror the same logic in meson I learned that the new
> logic removes the rpath setting from the parameters on at least netbsd and
> suse. We'll add them back - unless --disable-rpath is used.

Hmm ... on my shiny new netbsd buildfarm animal, I see:

$ perl -MConfig -e 'print "$Config{ccdlflags}"'
-Wl,-E  -Wl,-R/usr/pkg/lib/perl5/5.34.0/powerpc-netbsd-thread-multi/CORE

$ perl -MConfig -e 'print "$Config{ldflags}"'
 -pthread -L/usr/lib -Wl,-R/usr/lib -Wl,-R/usr/pkg/lib -L/usr/pkg/lib

$ perl -MExtUtils::Embed -e ldopts
-Wl,-E  -Wl,-R/usr/pkg/lib/perl5/5.34.0/powerpc-netbsd-thread-multi/CORE  -pthread -L/usr/lib -Wl,-R/usr/lib -Wl,-R/usr/pkg/lib -L/usr/pkg/lib  -L/usr/pkg/lib/perl5/5.34.0/powerpc-netbsd-thread-multi/CORE -lperl -lm -lcrypt -lpthread

So we were *already* stripping the rpath for where libperl.so is,
and now we also strip -L and rpath for /usr/lib (which surely is
pointless) and for /usr/pkg/lib (which in point of fact have to
be added to the PG configuration options anyway).  These Perl
options seem a bit inconsistent ...

> I think some
> distributions build with --disable-rpath. Not sure if worth worrying about.

I believe that policy only makes sense for distros that expect every
shared library to appear in /usr/lib (or /usr/lib64 perhaps).  Red Hat
for one does that --- but they follow through: libperl.so is there.

I think we're good here, at least till somebody points out a platform
where we aren't.

			regards, tom lane

  14. Re: Strip -mmacosx-version-min options from plperl build

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-24T10:12:15Z

    On 19.08.22 09:12, Peter Eisentraut wrote:
> After analyzing the source code of ExtUtils::Embed's ldopts, I think we 
> can also do this by subtracting $Config{ldflags}, since
> 
> my $linkage = "$ccdlflags $ldflags @archives $ld_or_bs";
> 
> and we really just want the $ld_or_bs part. (@archives should be empty 
> for our uses.)
> 
> This would get rid of -mmacosx-version-min and -arch and all the things 
> you showed, including -L/opt/local/lib, which is probably there so that 
> the build of Perl itself could look there for things, but we don't need it.

This patch has failed on Cygwin lorikeet:

Before:

checking for flags to link embedded Perl...
   -Wl,--enable-auto-import -Wl,--export-all-symbols 
-Wl,--enable-auto-image-base -fstack-protector-strong 
-L/usr/lib/perl5/5.32/x86_64-cygwin-threads/CORE -lperl -lpthread -ldl 
-lcrypt

After:

checking for flags to link embedded Perl... 
-L/usr/lib/perl5/5.32/x86_64-cygwin-threads/CORE -lperl -lpthread -ldl 
-lcrypt

That's as designed.  But the plperl tests fail:

CREATE EXTENSION plperl;
+ERROR:  incompatible library 
"/home/andrew/bf/root/HEAD/inst/lib/postgresql/plperl.dll": missing 
magic block
+HINT:  Extension libraries are required to use the PG_MODULE_MAGIC macro.

Among the now-dropped options, we can discount -Wl,--enable-auto-import, 
because that is used anyway via src/template/cygwin.

So one of the options

-Wl,--export-all-symbols
-Wl,--enable-auto-image-base
-fstack-protector-strong

is needed.  These options aren't used for any other shared libraries 
AFAICT, so nothing is clear to me.

  15. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-24T13:30:18Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> This patch has failed on Cygwin lorikeet:
> CREATE EXTENSION plperl;
> +ERROR:  incompatible library 
> "/home/andrew/bf/root/HEAD/inst/lib/postgresql/plperl.dll": missing 
> magic block

Presumably this is caused by not having

> -Wl,--export-all-symbols

which is something we ought to be injecting for ourselves if we
aren't doing anything to export the magic-block constant explicitly.
But I too am confused why we haven't seen this elsewhere.

			regards, tom lane

  16. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-24T14:24:50Z

    On 2022-08-24 We 09:30, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> This patch has failed on Cygwin lorikeet:
>> CREATE EXTENSION plperl;
>> +ERROR:  incompatible library 
>> "/home/andrew/bf/root/HEAD/inst/lib/postgresql/plperl.dll": missing 
>> magic block
> Presumably this is caused by not having
>
>> -Wl,--export-all-symbols
> which is something we ought to be injecting for ourselves if we
> aren't doing anything to export the magic-block constant explicitly.
> But I too am confused why we haven't seen this elsewhere.
>
> 			


Me too. I note that we have -Wl,--out-implib=libplperl.a but we don't
appear to do anything with libplperl.a.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  17. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-24T22:56:00Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2022-08-24 We 09:30, Tom Lane wrote:
>> Presumably this is caused by not having
>> > -Wl,--export-all-symbols
>> which is something we ought to be injecting for ourselves if we
>> aren't doing anything to export the magic-block constant explicitly.
>> But I too am confused why we haven't seen this elsewhere.

> Me too. I note that we have -Wl,--out-implib=libplperl.a but we don't
> appear to do anything with libplperl.a.

I've poked around and formed a vague theory, based on noting this
from the ld(1) man page:

       --export-all-symbols
           ... When symbols are
           explicitly exported via DEF files or implicitly exported via
           function attributes, the default is to not export anything else
           unless this option is given.

So we could explain the behavior if, say, plperl's _PG_init were
explicitly marked with __attribute__((visibility("default"))) while
its Pg_magic_func was not.  That would work anyway as long as
--export-all-symbols was being used at link time, and would produce
the observed symptom as soon as it wasn't.

Now, seeing that both of those functions are surely marked with
PGDLLEXPORT in the source code, how could such a state of affairs
arise?  What I'm thinking about is that _PG_init's marking will be
determined by the extern declaration for it in fmgr.h, while
Pg_magic_func's marking will be determined by the extern declaration
obtained from expanding PG_MODULE_MAGIC.  And there are a boatload
of Perl-specific header files read between those points in plperl.c.

In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
or somehow #define "__attribute__()" or "visibility()" into no-ops
(perhaps more likely) then we could explain this failure, and that
would also explain why it doesn't fail elsewhere.

I can't readily check this, since I have no idea exactly which version
of the Perl headers lorikeet uses.

			regards, tom lane

  18. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-25T00:14:27Z

    On 2022-08-24 We 18:56, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2022-08-24 We 09:30, Tom Lane wrote:
>>> Presumably this is caused by not having
>>>> -Wl,--export-all-symbols
>>> which is something we ought to be injecting for ourselves if we
>>> aren't doing anything to export the magic-block constant explicitly.
>>> But I too am confused why we haven't seen this elsewhere.
>> Me too. I note that we have -Wl,--out-implib=libplperl.a but we don't
>> appear to do anything with libplperl.a.
> I've poked around and formed a vague theory, based on noting this
> from the ld(1) man page:
>
>        --export-all-symbols
>            ... When symbols are
>            explicitly exported via DEF files or implicitly exported via
>            function attributes, the default is to not export anything else
>            unless this option is given.
>
> So we could explain the behavior if, say, plperl's _PG_init were
> explicitly marked with __attribute__((visibility("default"))) while
> its Pg_magic_func was not.  That would work anyway as long as
> --export-all-symbols was being used at link time, and would produce
> the observed symptom as soon as it wasn't.
>
> Now, seeing that both of those functions are surely marked with
> PGDLLEXPORT in the source code, how could such a state of affairs
> arise?  What I'm thinking about is that _PG_init's marking will be
> determined by the extern declaration for it in fmgr.h, while
> Pg_magic_func's marking will be determined by the extern declaration
> obtained from expanding PG_MODULE_MAGIC.  And there are a boatload
> of Perl-specific header files read between those points in plperl.c.
>
> In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
> or somehow #define "__attribute__()" or "visibility()" into no-ops
> (perhaps more likely) then we could explain this failure, and that
> would also explain why it doesn't fail elsewhere.
>
> I can't readily check this, since I have no idea exactly which version
> of the Perl headers lorikeet uses.
>
> 			



It's built against cygwin perl 5.32.


I don't see anything like that in perl.h. It's certainly using
__attribute__() a lot.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  19. Re: Strip -mmacosx-version-min options from plperl build

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-25T13:01:14Z

    On 25.08.22 02:14, Andrew Dunstan wrote:
>> In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
>> or somehow #define "__attribute__()" or "visibility()" into no-ops
>> (perhaps more likely) then we could explain this failure, and that
>> would also explain why it doesn't fail elsewhere.
>>
>> I can't readily check this, since I have no idea exactly which version
>> of the Perl headers lorikeet uses.
> 
> It's built against cygwin perl 5.32.
> 
> I don't see anything like that in perl.h. It's certainly using
> __attribute__() a lot.

This could be checked by running plperl.c through the preprocessor 
(replace gcc -c plperl.c -o plperl.o by gcc -E plperl.c -o plperl.i) and 
seeing what becomes of those symbols.

If we want to get the buildfarm green again sooner, we could force a 
--export-all-symbols directly.

  20. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-25T13:43:19Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>>> In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
>>> or somehow #define "__attribute__()" or "visibility()" into no-ops
>>> (perhaps more likely) then we could explain this failure, and that
>>> would also explain why it doesn't fail elsewhere.

> This could be checked by running plperl.c through the preprocessor 
> (replace gcc -c plperl.c -o plperl.o by gcc -E plperl.c -o plperl.i) and 
> seeing what becomes of those symbols.

Yeah, that was what I was going to suggest: grep the "-E" output for
_PG_init and Pg_magic_func and confirm what their extern declarations
look like.

> If we want to get the buildfarm green again sooner, we could force a 
> --export-all-symbols directly.

I'm not hugely upset as long as it's just the one machine failing.

			regards, tom lane

  21. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-25T21:39:35Z

    On 2022-08-25 Th 09:43, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>>>> In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
>>>> or somehow #define "__attribute__()" or "visibility()" into no-ops
>>>> (perhaps more likely) then we could explain this failure, and that
>>>> would also explain why it doesn't fail elsewhere.
>> This could be checked by running plperl.c through the preprocessor 
>> (replace gcc -c plperl.c -o plperl.o by gcc -E plperl.c -o plperl.i) and 
>> seeing what becomes of those symbols.
> Yeah, that was what I was going to suggest: grep the "-E" output for
> _PG_init and Pg_magic_func and confirm what their extern declarations
> look like.


$ egrep '_PG_init|Pg_magic_func'  plperl.i
extern __attribute__((visibility("default"))) void _PG_init(void);
extern __attribute__((visibility("default"))) const Pg_magic_struct
*Pg_magic_func(void); const Pg_magic_struct * Pg_magic_func(void) {
static const Pg_magic_struct Pg_magic_data = { sizeof(Pg_magic_struct),
160000 / 100, 100, 32, 64,
_PG_init(void)


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  22. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-25T21:47:39Z

    Hi,

On 2022-08-25 17:39:35 -0400, Andrew Dunstan wrote:
> On 2022-08-25 Th 09:43, Tom Lane wrote:
> > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> >>>> In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
> >>>> or somehow #define "__attribute__()" or "visibility()" into no-ops
> >>>> (perhaps more likely) then we could explain this failure, and that
> >>>> would also explain why it doesn't fail elsewhere.
> >> This could be checked by running plperl.c through the preprocessor 
> >> (replace gcc -c plperl.c -o plperl.o by gcc -E plperl.c -o plperl.i) and 
> >> seeing what becomes of those symbols.
> > Yeah, that was what I was going to suggest: grep the "-E" output for
> > _PG_init and Pg_magic_func and confirm what their extern declarations
> > look like.
> 
> 
> $ egrep '_PG_init|Pg_magic_func'  plperl.i
> extern __attribute__((visibility("default"))) void _PG_init(void);
> extern __attribute__((visibility("default"))) const Pg_magic_struct
> *Pg_magic_func(void); const Pg_magic_struct * Pg_magic_func(void) {
> static const Pg_magic_struct Pg_magic_data = { sizeof(Pg_magic_struct),
> 160000 / 100, 100, 32, 64,
> _PG_init(void)

Could you show objdump -t of the library? Perhaps once with the flags as now,
and once relinking with the "old" flags that we're now omitting?

Greetings,

Andres Freund

  23. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-25T22:04:34Z

    On 2022-08-25 Th 17:47, Andres Freund wrote:
> Hi,
>
> On 2022-08-25 17:39:35 -0400, Andrew Dunstan wrote:
>> On 2022-08-25 Th 09:43, Tom Lane wrote:
>>> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>>>>>> In short: if the Cygwin Perl headers redefine PGDLLEXPORT (unlikely)
>>>>>> or somehow #define "__attribute__()" or "visibility()" into no-ops
>>>>>> (perhaps more likely) then we could explain this failure, and that
>>>>>> would also explain why it doesn't fail elsewhere.
>>>> This could be checked by running plperl.c through the preprocessor 
>>>> (replace gcc -c plperl.c -o plperl.o by gcc -E plperl.c -o plperl.i) and 
>>>> seeing what becomes of those symbols.
>>> Yeah, that was what I was going to suggest: grep the "-E" output for
>>> _PG_init and Pg_magic_func and confirm what their extern declarations
>>> look like.
>>
>> $ egrep '_PG_init|Pg_magic_func'  plperl.i
>> extern __attribute__((visibility("default"))) void _PG_init(void);
>> extern __attribute__((visibility("default"))) const Pg_magic_struct
>> *Pg_magic_func(void); const Pg_magic_struct * Pg_magic_func(void) {
>> static const Pg_magic_struct Pg_magic_data = { sizeof(Pg_magic_struct),
>> 160000 / 100, 100, 32, 64,
>> _PG_init(void)
> Could you show objdump -t of the library? Perhaps once with the flags as now,
> and once relinking with the "old" flags that we're now omitting?


current:


$ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
[103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040a0
Pg_magic_func
[105](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040b0 _PG_init


from July 11th build:


$ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
[101](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040d0
Pg_magic_func
[103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040e0 _PG_init


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  24. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-25T22:13:25Z

    Hi,

On 2022-08-25 18:04:34 -0400, Andrew Dunstan wrote:
> On 2022-08-25 Th 17:47, Andres Freund wrote:
> >> $ egrep '_PG_init|Pg_magic_func'  plperl.i
> >> extern __attribute__((visibility("default"))) void _PG_init(void);
> >> extern __attribute__((visibility("default"))) const Pg_magic_struct
> >> *Pg_magic_func(void); const Pg_magic_struct * Pg_magic_func(void) {
> >> static const Pg_magic_struct Pg_magic_data = { sizeof(Pg_magic_struct),
> >> 160000 / 100, 100, 32, 64,
> >> _PG_init(void)
> > Could you show objdump -t of the library? Perhaps once with the flags as now,
> > and once relinking with the "old" flags that we're now omitting?
> 
> 
> current:
> 
> 
> $ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
> [103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040a0
> Pg_magic_func
> [105](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040b0 _PG_init
> 
> 
> from July 11th build:
> 
> 
> $ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
> [101](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040d0
> Pg_magic_func
> [103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040e0 _PG_init

Thanks.

So it looks like it's not the symbol not being exported. I wonder if the image
base thing is somehow the problem? Sounds like it should just be an efficiency
difference, by avoiding some relocations, not a functional difference.

Can you try adding just that to the flags for building and whether that then
allows a LOAD 'plperl' to succeed?

Greetings,

Andres Freund

  25. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-26T14:04:35Z

    On 2022-08-25 Th 18:13, Andres Freund wrote:
> Hi,
>
> On 2022-08-25 18:04:34 -0400, Andrew Dunstan wrote:
>> On 2022-08-25 Th 17:47, Andres Freund wrote:
>>>> $ egrep '_PG_init|Pg_magic_func'  plperl.i
>>>> extern __attribute__((visibility("default"))) void _PG_init(void);
>>>> extern __attribute__((visibility("default"))) const Pg_magic_struct
>>>> *Pg_magic_func(void); const Pg_magic_struct * Pg_magic_func(void) {
>>>> static const Pg_magic_struct Pg_magic_data = { sizeof(Pg_magic_struct),
>>>> 160000 / 100, 100, 32, 64,
>>>> _PG_init(void)
>>> Could you show objdump -t of the library? Perhaps once with the flags as now,
>>> and once relinking with the "old" flags that we're now omitting?
>>
>> current:
>>
>>
>> $ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
>> [103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040a0
>> Pg_magic_func
>> [105](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040b0 _PG_init
>>
>>
>> from July 11th build:
>>
>>
>> $ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
>> [101](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040d0
>> Pg_magic_func
>> [103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040e0 _PG_init
> Thanks.
>
> So it looks like it's not the symbol not being exported. I wonder if the image
> base thing is somehow the problem? Sounds like it should just be an efficiency
> difference, by avoiding some relocations, not a functional difference.
>
> Can you try adding just that to the flags for building and whether that then
> allows a LOAD 'plperl' to succeed?
>


Adding what?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  26. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-26T14:14:32Z

    Hi,

On 2022-08-26 10:04:35 -0400, Andrew Dunstan wrote:
> On 2022-08-25 Th 18:13, Andres Freund wrote:
> >>> Could you show objdump -t of the library? Perhaps once with the flags as now,
> >>> and once relinking with the "old" flags that we're now omitting?
> >>
> >> current:
> >>
> >>
> >> $ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
> >> [103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040a0
> >> Pg_magic_func
> >> [105](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040b0 _PG_init
> >>
> >>
> >> from July 11th build:
> >>
> >>
> >> $ objdump -t plperl.dll | egrep '_PG_init|Pg_magic_func'
> >> [101](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040d0
> >> Pg_magic_func
> >> [103](sec  1)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000000040e0 _PG_init
> > Thanks.
> >
> > So it looks like it's not the symbol not being exported. I wonder if the image
> > base thing is somehow the problem? Sounds like it should just be an efficiency
> > difference, by avoiding some relocations, not a functional difference.
> >
> > Can you try adding just that to the flags for building and whether that then
> > allows a LOAD 'plperl' to succeed?
> >
> 
> 
> Adding what?

-Wl,--enable-auto-image-base

Greetings,

Andres Freund

  27. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-26T16:11:42Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-08-26 10:04:35 -0400, Andrew Dunstan wrote:
>> On 2022-08-25 Th 18:13, Andres Freund wrote:
>>> Can you try adding just that to the flags for building and whether that then
>>> allows a LOAD 'plperl' to succeed?

>> Adding what?

> -Wl,--enable-auto-image-base

And if that doesn't help, try -Wl,--export-all-symbols

			regards, tom lane

  28. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-26T19:36:16Z

    On 2022-08-26 Fr 12:11, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2022-08-26 10:04:35 -0400, Andrew Dunstan wrote:
>>> On 2022-08-25 Th 18:13, Andres Freund wrote:
>>>> Can you try adding just that to the flags for building and whether that then
>>>> allows a LOAD 'plperl' to succeed?
>>> Adding what?
>> -Wl,--enable-auto-image-base


didn't work


> And if that doesn't help, try -Wl,--export-all-symbols


worked


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  29. Re: Strip -mmacosx-version-min options from plperl build

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-26T20:00:31Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2022-08-26 Fr 12:11, Tom Lane wrote:
>> And if that doesn't help, try -Wl,--export-all-symbols

> worked

Hmph.  Hard to see how that isn't a linker bug.  As a stopgap
to get the farm green again, I propose adding something like

ifeq ($(PORTNAME), cygwin)
SHLIB_LINK += -Wl,--export-all-symbols
endif

to plperl's makefile.

			regards, tom lane

  30. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-26T20:07:32Z

    On 2022-08-26 Fr 16:00, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2022-08-26 Fr 12:11, Tom Lane wrote:
>>> And if that doesn't help, try -Wl,--export-all-symbols
>> worked
> Hmph.  Hard to see how that isn't a linker bug.  As a stopgap
> to get the farm green again, I propose adding something like
>
> ifeq ($(PORTNAME), cygwin)
> SHLIB_LINK += -Wl,--export-all-symbols
> endif
>
> to plperl's makefile.
>
> 			


+1


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  31. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-08-26T20:25:41Z

    Hi,

On 2022-08-26 16:00:31 -0400, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 2022-08-26 Fr 12:11, Tom Lane wrote:
> >> And if that doesn't help, try -Wl,--export-all-symbols
>
> > worked

Except that it's only happening for plperl, I'd wonder if it's possibly
related to our magic symbols being prefixed with _. I noticed that the
underscore prefix e.g. changes the behaviour of gcc's "collect2" on AIX, which
is responsible for exporting symbols etc.


> Hmph.  Hard to see how that isn't a linker bug.

Agreed, given that this is only happening with plperl, and not with any of the
other extensions...


> As a stopgap to get the farm green again, I propose adding something like
>
> ifeq ($(PORTNAME), cygwin)
> SHLIB_LINK += -Wl,--export-all-symbols
> endif
>
> to plperl's makefile.

:(

Greetings,

Andres Freund

  32. Re: Strip -mmacosx-version-min options from plperl build

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-30T13:35:51Z

    On 2022-08-26 Fr 16:25, Andres Freund wrote:
> Hi,
>
> On 2022-08-26 16:00:31 -0400, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> On 2022-08-26 Fr 12:11, Tom Lane wrote:
>>>> And if that doesn't help, try -Wl,--export-all-symbols
>>> worked
> Except that it's only happening for plperl, I'd wonder if it's possibly
> related to our magic symbols being prefixed with _. I noticed that the
> underscore prefix e.g. changes the behaviour of gcc's "collect2" on AIX, which
> is responsible for exporting symbols etc.
>
>
>> Hmph.  Hard to see how that isn't a linker bug.
> Agreed, given that this is only happening with plperl, and not with any of the
> other extensions...
>
>
>> As a stopgap to get the farm green again, I propose adding something like
>>
>> ifeq ($(PORTNAME), cygwin)
>> SHLIB_LINK += -Wl,--export-all-symbols
>> endif
>>
>> to plperl's makefile.
> :(
>

It doesn't make me very happy either, but nobody seems to have a better
idea.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  33. Re: Strip -mmacosx-version-min options from plperl build

    Andres Freund <andres@anarazel.de> — 2022-09-28T02:52:42Z

    Hi,

On 2022-08-30 09:35:51 -0400, Andrew Dunstan wrote:
> On 2022-08-26 Fr 16:25, Andres Freund wrote:
> > On 2022-08-26 16:00:31 -0400, Tom Lane wrote:
> >> Andrew Dunstan <andrew@dunslane.net> writes:
> >>> On 2022-08-26 Fr 12:11, Tom Lane wrote:
> >>>> And if that doesn't help, try -Wl,--export-all-symbols
> >>> worked
> > Except that it's only happening for plperl, I'd wonder if it's possibly
> > related to our magic symbols being prefixed with _. I noticed that the
> > underscore prefix e.g. changes the behaviour of gcc's "collect2" on AIX, which
> > is responsible for exporting symbols etc.
> >
> >
> >> Hmph.  Hard to see how that isn't a linker bug.
> > Agreed, given that this is only happening with plperl, and not with any of the
> > other extensions...
> >
> >
> >> As a stopgap to get the farm green again, I propose adding something like
> >>
> >> ifeq ($(PORTNAME), cygwin)
> >> SHLIB_LINK += -Wl,--export-all-symbols
> >> endif
> >>
> >> to plperl's makefile.
> > :(
> >
>
> It doesn't make me very happy either, but nobody seems to have a better
> idea.

The plpython issue I was investigating in
https://postgr.es/m/20220928022724.erzuk5v4ai4b53do%40awork3.anarazel.de
feels eerily similar to the issue here.

I wonder if it's the same problem - __attribute__((visibility("default")))
works to export - unless another symbol uses __declspec (dllexport). In the
referenced thread that was PyInit_plpy(), here it could be some perl generated
one.

Does this issue resolved if you add
#define PGDLLEXPORT __declspec (dllexport)
to cygwin.h?  Without the -Wl,--export-all-symbols of course.

Greetings,

Andres Freund