Обсуждение: Annoying build warnings from latest Apple toolchain

Since updating to Xcode 15.0, my macOS machines have been
spitting a bunch of linker-generated warnings.  There
seems to be one instance of

ld: warning: -multiply_defined is obsolete

for each loadable module we link, and some program links complain

ld: warning: ignoring duplicate libraries: '-lpgcommon', '-lpgport'

You can see these in the build logs for both sifaka [1] and
indri [2], so MacPorts isn't affecting it.

I'd held out some hope that this was a transient problem due to
the underlying OS still being Ventura, but I just updated another
machine to shiny new Sonoma (14.0), and it's still doing it.
Guess we gotta do something about it.

We used to need "-multiply_defined suppress" to suppress other
linker warnings.  I tried removing that from the Makefile.shlib
recipes for Darwin, and those complaints go away while no new
ones appear, so that's good --- but I wonder whether slightly
older toolchain versions will still want it.

As for the duplicate libraries, yup guilty as charged, but
I think we were doing that intentionally to satisfy some other
old toolchains.  I wonder whether removing the duplication
will create new problems.

            regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=sifaka&dt=2023-09-26%2021%3A09%3A01&stg=build
[2] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=indri&dt=2023-09-26%2021%3A42%3A17&stg=build

I wrote:
> Since updating to Xcode 15.0, my macOS machines have been
> spitting a bunch of linker-generated warnings.  There
> seems to be one instance of
> ld: warning: -multiply_defined is obsolete
> for each loadable module we link ...

I poked into this a little more.  We started using "-multiply_defined
suppress" in commit 9df308697 of 2004-07-13, which was in the OS X 10.3
era.  I failed to find any specific discussion of that switch in our
archives, but the commit message suggests that I probably stole it
from a patch the Fink project was carrying.

Googling finds some non-authoritative claims that "-multiply_defined"
has been a no-op since OS X 10.9 (Mavericks).  I don't have anything
older than 10.15 to check, but removing it on 10.15 does not seem
to cause any problems.

So I think we can safely just remove this switch from Makefile.shlib.
The meson build process isn't invoking it either I think.

The other thing will take a bit more work ...

            regards, tom lane

I wrote:
> Since updating to Xcode 15.0, my macOS machines have been
> spitting a bunch of linker-generated warnings. ...
> some program links complain

> ld: warning: ignoring duplicate libraries: '-lpgcommon', '-lpgport'

I found that this is being caused by the libpq_pgport hack in
Makefile.global.in, which ensures that libpgcommon and libpgport
get linked before libpq.  The comment freely admits that it results in
linking libpgcommon and libpgport twice.  Now, AFAICS that whole hack
is unnecessary on any platform where we know how to do symbol export
control, because then libpq won't expose any of the troublesome
symbols to begin with.  So we can resolve the problem by just not
doing that on macOS, as in the attached draft patch.  I've confirmed
that this suppresses the duplicate-libraries warnings on Xcode 15.0
without creating any issues on older macOS (though I'm only in a
position to test as far back as Catalina).

The patch is written to change things only on macOS, but I wonder
if we should be more aggressive and change it for all platforms
where we have symbol export control (which is almost everything
these days).  I doubt it'd make any noticeable difference in
build time, but doing that would give us more test coverage
which would help expose any weak spots.

I've not yet looked at the meson build infrastructure to
see if it needs a corresponding change.

            regards, tom lane

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 18240b5fef..cded651755 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -589,19 +589,31 @@ endif
 libpq = -L$(libpq_builddir) -lpq

 # libpq_pgport is for use by client executables (not libraries) that use libpq.
-# We force clients to pull symbols from the non-shared libraries libpgport
+# We want clients to pull symbols from the non-shared libraries libpgport
 # and libpgcommon rather than pulling some libpgport symbols from libpq just
 # because libpq uses those functions too.  This makes applications less
-# dependent on changes in libpq's usage of pgport (on platforms where we
-# don't have symbol export control for libpq).  To do this we link to
+# dependent on changes in libpq's usage of pgport.  To do this we link to
 # pgport before libpq.  This does cause duplicate -lpgport's to appear
-# on client link lines, since that also appears in $(LIBS).
+# on client link lines, since that also appears in $(LIBS).  On platforms
+# where we have symbol export control for libpq, the whole exercise is
+# unnecessary because libpq won't expose any of these symbols.  Currently,
+# only macOS warns about duplicate library references, so we only suppress
+# the duplicates on macOS.
+ifeq ($(PORTNAME),darwin)
+libpq_pgport = $(libpq)
+else ifdef PGXS
+libpq_pgport = -L$(libdir) -lpgcommon -lpgport $(libpq)
+else
+libpq_pgport = -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/port -lpgport $(libpq)
+endif
+
 # libpq_pgport_shlib is the same idea, but for use in client shared libraries.
+# We need those clients to use the shlib variants, even on macOS.  (Ideally,
+# users of this macro would strip libpgport and libpgcommon from $(LIBS),
+# but no harm is done if they don't.)
 ifdef PGXS
-libpq_pgport = -L$(libdir) -lpgcommon -lpgport $(libpq)
 libpq_pgport_shlib = -L$(libdir) -lpgcommon_shlib -lpgport_shlib $(libpq)
 else
-libpq_pgport = -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/port -lpgport $(libpq)
 libpq_pgport_shlib = -L$(top_builddir)/src/common -lpgcommon_shlib -L$(top_builddir)/src/port -lpgport_shlib $(libpq)
 endif

I wrote:
> I've not yet looked at the meson build infrastructure to
> see if it needs a corresponding change.

I think it doesn't, as long as all the relevant build targets
write their dependencies with "frontend_code" before "libpq".
(The need for this is, of course, documented nowhere.  The state
of the documentation for our meson build system is abysmal.)

However, it's hard to test this, because the meson build
seems completely broken on current macOS:

-----
$ meson setup build --prefix=$HOME/pginstall
The Meson build system
Version: 0.64.1
Source dir: /Users/tgl/pgsql
Build dir: /Users/tgl/pgsql/build
Build type: native build
Project name: postgresql
Project version: 17devel

meson.build:9:0: ERROR: Unable to detect linker for compiler `cc -Wl,--version`
stdout:
stderr: ld: unknown options: --version
clang: error: linker command failed with exit code 1 (use -v to see invocation)

A full log can be found at /Users/tgl/pgsql/build/meson-logs/meson-log.txt
-----

That log file offers no more detail than the terminal output did.

(I also tried with a more recent meson version, 1.1.1, with
the same result.)

            regards, tom lane

Hi,

On 2023-09-27 16:52:44 -0400, Tom Lane wrote:
> I wrote:
> > I've not yet looked at the meson build infrastructure to
> > see if it needs a corresponding change.
>
> I think it doesn't, as long as all the relevant build targets
> write their dependencies with "frontend_code" before "libpq".

Hm, that's not great. I don't think that should be required. I'll try to take
a look at why that's needed.


> However, it's hard to test this, because the meson build
> seems completely broken on current macOS:

I am travelling and I don't quite dare to upgrade my mac mini remotely. So I
can't try Sonoma directly.

But CI worked after switching to sonoma - although installing packages from
macports took forever, due to macports building all packages locally.

https://cirrus-ci.com/task/5133869171605504

There's some weird warnings about hashlib/blake2, but it looks like that's a
python installation issue. Looks like this is with python from macports in
PATH.

[00:59:14.442] ERROR:root:code for hash blake2b was not found.
[00:59:14.442] Traceback (most recent call last):
[00:59:14.442]   File "/opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/hashlib.py", line
307,in <module> 
[00:59:14.442]     globals()[__func_name] = __get_hash(__func_name)
[00:59:14.442]                              ^^^^^^^^^^^^^^^^^^^^^^^
[00:59:14.442]   File "/opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/hashlib.py", line
129,in __get_openssl_constructor 
[00:59:14.442]     return __get_builtin_constructor(name)
[00:59:14.442]            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:59:14.442]   File "/opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/hashlib.py", line
123,in __get_builtin_constructor 
[00:59:14.442]     raise ValueError('unsupported hash type ' + name)
[00:59:14.442] ValueError: unsupported hash type blake2b

This just happens whenever python's hashlib - supposedly in the standard
library - is imported.


There *are* some buildsystem warnings:
[00:59:27.289] [260/2328] Linking target src/interfaces/libpq/libpq.5.dylib
[00:59:27.289] ld: warning: -undefined error is deprecated
[00:59:27.289] ld: warning: ignoring -e, not used for output type

Full command:
[1/1] cc  -o src/interfaces/libpq/libpq.5.dylib src/interfaces/libpq/libpq.5.dylib.p/fe-auth-scram.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-auth.c.osrc/interfaces/libpq/libpq.5.dylib.p/fe-connect.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-exec.c.osrc/interfaces/libpq/libpq.5.dylib.p/fe-lobj.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-misc.c.osrc/interfaces/libpq/libpq.5.dylib.p/fe-print.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-protocol3.c.osrc/interfaces/libpq/libpq.5.dylib.p/fe-secure.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-trace.c.osrc/interfaces/libpq/libpq.5.dylib.p/legacy-pqsignal.c.o
src/interfaces/libpq/libpq.5.dylib.p/libpq-events.c.osrc/interfaces/libpq/libpq.5.dylib.p/pqexpbuffer.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-secure-common.c.osrc/interfaces/libpq/libpq.5.dylib.p/fe-secure-openssl.c.o
src/interfaces/libpq/libpq.5.dylib.p/fe-gssapi-common.c.osrc/interfaces/libpq/libpq.5.dylib.p/fe-secure-gssapi.c.o
-Wl,-dead_strip_dylibs-Wl,-headerpad_max_install_names -Wl,-undefined,error -shared -install_name @rpath/libpq.5.dylib
-compatibility_version5 -current_version 5.17 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk -Og
-ggdb-Wl,-rpath,/opt/local/lib -Wl,-rpath,/opt/local/libexec/openssl3/lib src/common/libpgcommon_shlib.a
src/port/libpgport_shlib.a-exported_symbols_list=/Users/admin/pgsql/build/src/interfaces/libpq/exports.list -lm
/opt/local/lib/libintl.dylib/opt/local/lib/libgssapi_krb5.dylib /opt/local/lib/libldap.dylib
/opt/local/lib/liblber.dylib/opt/local/libexec/openssl3/lib/libssl.dylib
/opt/local/libexec/openssl3/lib/libcrypto.dylib/opt/local/lib/libz.dylib /opt/local/lib/libzstd.dylib 
ld: warning: -undefined error is deprecated
ld: warning: ignoring -e, not used for output type

So we need to make the addition of -Wl,-undefined,error conditional, that
should be easy enough. Although I'm a bit confused about this being
deprecated.

For the -e bit, this seems to do the trick:
@@ -224,7 +224,7 @@ elif host_system == 'darwin'
   library_path_var = 'DYLD_LIBRARY_PATH'

   export_file_format = 'darwin'
-  export_fmt = '-exported_symbols_list=@0@'
+  export_fmt = '-Wl,-exported_symbols_list,@0@'

   mod_link_args_fmt = ['-bundle_loader', '@0@']
   mod_link_with_dir = 'bindir'

It's quite annoying that apple is changing things option syntax.


> (I also tried with a more recent meson version, 1.1.1, with
> the same result.)

Looks like you need 1.2 for the new clang / ld output...  Apparently apple's
linker changed the format of its version output :/.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-09-27 16:52:44 -0400, Tom Lane wrote:
>> I think it doesn't, as long as all the relevant build targets
>> write their dependencies with "frontend_code" before "libpq".

> Hm, that's not great. I don't think that should be required. I'll try to take
> a look at why that's needed.

Well, it's only important on platforms where we can't restrict
libpq.so from exporting all symbols.  I don't know how close we are
to deciding that such cases are no longer interesting to worry about.
Makefile.shlib seems to know how to do it everywhere except Windows,
and I imagine we know how to do it over in the MSVC scripts.

>> However, it's hard to test this, because the meson build
>> seems completely broken on current macOS:

> Looks like you need 1.2 for the new clang / ld output...  Apparently apple's
> linker changed the format of its version output :/.

Ah, yeah, updating MacPorts again brought in meson 1.2.1 which seems
to work.  I now see a bunch of

ld: warning: ignoring -e, not used for output type
ld: warning: -undefined error is deprecated

which are unrelated.  There's still one duplicate warning
from the backend link:

ld: warning: ignoring duplicate libraries: '-lpam'

I'm a bit baffled why that's showing up; there's no obvious
double reference to pam.

            regards, tom lane

Hi,

On 2023-09-28 16:46:08 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-09-27 16:52:44 -0400, Tom Lane wrote:
> >> I think it doesn't, as long as all the relevant build targets
> >> write their dependencies with "frontend_code" before "libpq".
> 
> > Hm, that's not great. I don't think that should be required. I'll try to take
> > a look at why that's needed.
> 
> Well, it's only important on platforms where we can't restrict
> libpq.so from exporting all symbols.  I don't know how close we are
> to deciding that such cases are no longer interesting to worry about.
> Makefile.shlib seems to know how to do it everywhere except Windows,
> and I imagine we know how to do it over in the MSVC scripts.

Hm, then I'd argue that we don't need to care about it anymore. The meson
build does the necessary magic on windows, as do the current msvc scripts.

I think right now it doesn't work as-is on sonoma, because apple decided to
change the option syntax, which is what causes the -e warning below, so the
relevant option is just ignored.


> There's still one duplicate warning
> from the backend link:
> 
> ld: warning: ignoring duplicate libraries: '-lpam'
> 
> I'm a bit baffled why that's showing up; there's no obvious
> double reference to pam.

I think it's because multiple libraries/binaries depend on it. Meson knows how
to deduplicate libraries found via pkg-config (presumably because that has
enough information for a topological sort), but apparently not when they're
found as "raw" libraries.  Until now that was also just pretty harmless, so I
understand not doing anything about it.

I see a way to avoid the warnings, but perhaps it's better to ask the meson
folks to put in a generic way of dealing with this.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-09-28 16:46:08 -0400, Tom Lane wrote:
>> Well, it's only important on platforms where we can't restrict
>> libpq.so from exporting all symbols.  I don't know how close we are
>> to deciding that such cases are no longer interesting to worry about.
>> Makefile.shlib seems to know how to do it everywhere except Windows,
>> and I imagine we know how to do it over in the MSVC scripts.

> Hm, then I'd argue that we don't need to care about it anymore. The meson
> build does the necessary magic on windows, as do the current msvc scripts.

If we take that argument seriously, then I'm inclined to adjust my
upthread patch for Makefile.global.in so that it removes the extra
inclusions of libpgport/libpgcommon everywhere, not only macOS.
The rationale would be that it's not worth worrying about ABI
stability details on any straggler platforms.

> I think right now it doesn't work as-is on sonoma, because apple decided to
> change the option syntax, which is what causes the -e warning below, so the
> relevant option is just ignored.

Hmm, we'd better fix that then.  Or is it their bug?  It looks to me like
clang's argument is -exported_symbols_list=/path/to/exports.list, so
it must be translating that to "-e".

            regards, tom lane

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> I think right now it doesn't work as-is on sonoma, because apple decided to
>> change the option syntax, which is what causes the -e warning below, so the
>> relevant option is just ignored.

> Hmm, we'd better fix that then.  Or is it their bug?  It looks to me like
> clang's argument is -exported_symbols_list=/path/to/exports.list, so
> it must be translating that to "-e".

Looking closer, the documented syntax is

    -exported_symbols_list filename

(two arguments, not one with an "=").  That is what our Makefiles
use, and it still works fine with latest Xcode.  However, meson.build
thinks it can get away with one argument containing "=", and evidently
that doesn't work now (or maybe it never did?).

I tried

  export_fmt = '-exported_symbols_list @0@'

and

  export_fmt = ['-exported_symbols_list', '@0@']

and neither of those did what I wanted, so maybe I will have to
study meson's command language sometime soon.  In the meantime,
I suppose this might be an easy fix for somebody who knows their
way around meson.

            regards, tom lane

Hi,

On 2023-09-28 19:17:37 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> I think right now it doesn't work as-is on sonoma, because apple decided to
> >> change the option syntax, which is what causes the -e warning below, so the
> >> relevant option is just ignored.
> 
> > Hmm, we'd better fix that then.  Or is it their bug?  It looks to me like
> > clang's argument is -exported_symbols_list=/path/to/exports.list, so
> > it must be translating that to "-e".
> 
> Looking closer, the documented syntax is
> 
>     -exported_symbols_list filename
> 
> (two arguments, not one with an "=").  That is what our Makefiles
> use, and it still works fine with latest Xcode.  However, meson.build
> thinks it can get away with one argument containing "=", and evidently
> that doesn't work now (or maybe it never did?).

It does still work on Ventura.


> I tried
> 
>   export_fmt = '-exported_symbols_list @0@'

That would expand to a single argument with a space inbetween.


> and
> 
>   export_fmt = ['-exported_symbols_list', '@0@']

That would work in many places, but not here, export_fmt is used as a format
string... We could make the callsites do that for each array element, but
there's an easier solution that seems to work for both Ventura and Sonoma -
however I don't have anything older to test with.

TBH, I find it hard to understand what arguments go to the linker and which to
the compiler on macos. The argument is documented for the linker and not the
compiler, but so far we'd been passing it to the compiler, so there must be
some logic forwarding it.

Looking through the clang code, I see various llvm libraries using
-Wl,-exported_symbols_list and there are tests
(clang/test/Driver/darwin-ld.c) ensuring both syntaxes work.

Thus the easiest fix looks to be to use this:

diff --git a/meson.build b/meson.build
index 5422885b0a2..16a2b0f801e 100644
--- a/meson.build
+++ b/meson.build
@@ -224,7 +224,7 @@ elif host_system == 'darwin'
   library_path_var = 'DYLD_LIBRARY_PATH'
 
   export_file_format = 'darwin'
-  export_fmt = '-exported_symbols_list=@0@'
+  export_fmt = '-Wl,-exported_symbols_list,@0@'
 
   mod_link_args_fmt = ['-bundle_loader', '@0@']
   mod_link_with_dir = 'bindir'


I don't have anything older than Ventura to check though.

Greetings,

Andres Freund

Hi,

On 2023-09-28 19:20:27 -0700, Andres Freund wrote:
> Thus the easiest fix looks to be to use this:
> 
> diff --git a/meson.build b/meson.build
> index 5422885b0a2..16a2b0f801e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -224,7 +224,7 @@ elif host_system == 'darwin'
>    library_path_var = 'DYLD_LIBRARY_PATH'
>  
>    export_file_format = 'darwin'
> -  export_fmt = '-exported_symbols_list=@0@'
> +  export_fmt = '-Wl,-exported_symbols_list,@0@'
>  
>    mod_link_args_fmt = ['-bundle_loader', '@0@']
>    mod_link_with_dir = 'bindir'
> 
> 
> I don't have anything older than Ventura to check though.

Attached is the above change and a commit to change CI over to Sonoma. Not
sure when we should switch, but it seems useful to include for testing
purposes at the very least.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-09-28 19:20:27 -0700, Andres Freund wrote:
>> Thus the easiest fix looks to be to use this:
>> -  export_fmt = '-exported_symbols_list=@0@'
>> +  export_fmt = '-Wl,-exported_symbols_list,@0@'
>> I don't have anything older than Ventura to check though.

I don't have meson installed on my surviving Catalina box, but
I tried the equivalent thing in the Makefile universe:

diff --git a/src/Makefile.shlib b/src/Makefile.shlib
index f94d59d1c5..f2ed222cc7 100644
--- a/src/Makefile.shlib
+++ b/src/Makefile.shlib
@@ -136,7 +136,7 @@ ifeq ($(PORTNAME), darwin)
   BUILD.exports                = $(AWK) '/^[^\#]/ {printf "_%s\n",$$1}' $< >$@
   exports_file         = $(SHLIB_EXPORTS:%.txt=%.list)
   ifneq (,$(exports_file))
-    exported_symbols_list = -exported_symbols_list $(exports_file)
+    exported_symbols_list = -Wl,-exported_symbols_list,$(exports_file)
   endif
 endif

That builds and produces correctly-symbol-trimmed shlibs, so I'd
say it's fine.  (Perhaps we should apply the above to HEAD
alongside the meson.build fix, to get more test coverage?)

> Attached is the above change and a commit to change CI over to Sonoma. Not
> sure when we should switch, but it seems useful to include for testing
> purposes at the very least.

No opinion on when to switch CI.  Sonoma is surely pretty bleeding edge
yet.

            regards, tom lane

Hi,

On 2023-09-28 22:53:09 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-09-28 19:20:27 -0700, Andres Freund wrote:
> >> Thus the easiest fix looks to be to use this:
> >> -  export_fmt = '-exported_symbols_list=@0@'
> >> +  export_fmt = '-Wl,-exported_symbols_list,@0@'
> >> I don't have anything older than Ventura to check though.
> 
> I don't have meson installed on my surviving Catalina box, but
> I tried the equivalent thing in the Makefile universe:
> 
> diff --git a/src/Makefile.shlib b/src/Makefile.shlib
> index f94d59d1c5..f2ed222cc7 100644
> --- a/src/Makefile.shlib
> +++ b/src/Makefile.shlib
> @@ -136,7 +136,7 @@ ifeq ($(PORTNAME), darwin)
>    BUILD.exports                = $(AWK) '/^[^\#]/ {printf "_%s\n",$$1}' $< >$@
>    exports_file         = $(SHLIB_EXPORTS:%.txt=%.list)
>    ifneq (,$(exports_file))
> -    exported_symbols_list = -exported_symbols_list $(exports_file)
> +    exported_symbols_list = -Wl,-exported_symbols_list,$(exports_file)
>    endif
>  endif
> 
> That builds and produces correctly-symbol-trimmed shlibs, so I'd
> say it's fine.

Thanks for testing!

I'll go and push that 16/HEAD then.


> (Perhaps we should apply the above to HEAD alongside the meson.build fix, to
> get more test coverage?)

The macos animals BF seem to run Ventura, so I think it'd not really provide
additional coverage that CI and your manual testing already has. So probably
not worth it from that angle?


> > Attached is the above change and a commit to change CI over to Sonoma. Not
> > sure when we should switch, but it seems useful to include for testing
> > purposes at the very least.
> 
> No opinion on when to switch CI.  Sonoma is surely pretty bleeding edge
> yet.

Yea, it does feel like that...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-09-28 22:53:09 -0400, Tom Lane wrote:
>> (Perhaps we should apply the above to HEAD alongside the meson.build fix, to
>> get more test coverage?)

> The macos animals BF seem to run Ventura, so I think it'd not really provide
> additional coverage that CI and your manual testing already has. So probably
> not worth it from that angle?

My thought was that if it's in the tree we'd get testing from
non-buildfarm sources.

FWIW, I'm going to update sifaka/indri to Sonoma before too much longer
(they're already using Xcode 15.0 which is the Sonoma toolchain).
I recently pulled longfin up to Ventura, and plan to leave it on that
for the next year or so.  I don't think anyone else is running macOS
animals.

            regards, tom lane

On Thu Sep 28, 2023 at 5:22 PM CDT, Andres Freund wrote:
> Hi,
>
> On 2023-09-28 16:46:08 -0400, Tom Lane wrote:
> > There's still one duplicate warning
> > from the backend link:
> >
> > ld: warning: ignoring duplicate libraries: '-lpam'
> >
> > I'm a bit baffled why that's showing up; there's no obvious
> > double reference to pam.
>
> I think it's because multiple libraries/binaries depend on it. Meson knows how
> to deduplicate libraries found via pkg-config (presumably because that has
> enough information for a topological sort), but apparently not when they're
> found as "raw" libraries.  Until now that was also just pretty harmless, so I
> understand not doing anything about it.
>
> I see a way to avoid the warnings, but perhaps it's better to ask the meson
> folks to put in a generic way of dealing with this.

I wonder if this Meson PR[0] will help.

[0]: https://github.com/mesonbuild/meson/pull/12276

--
Tristan Partin
Neon (https://neon.tech)

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2023-09-28 16:46:08 -0400, Tom Lane wrote:
>>> Well, it's only important on platforms where we can't restrict
>>> libpq.so from exporting all symbols.  I don't know how close we are
>>> to deciding that such cases are no longer interesting to worry about.
>>> Makefile.shlib seems to know how to do it everywhere except Windows,
>>> and I imagine we know how to do it over in the MSVC scripts.

>> Hm, then I'd argue that we don't need to care about it anymore. The meson
>> build does the necessary magic on windows, as do the current msvc scripts.

> If we take that argument seriously, then I'm inclined to adjust my
> upthread patch for Makefile.global.in so that it removes the extra
> inclusions of libpgport/libpgcommon everywhere, not only macOS.
> The rationale would be that it's not worth worrying about ABI
> stability details on any straggler platforms.

Looking closer, it's only since v16 that we have export list support
on all officially-supported platforms.  Therefore, I think the prudent
thing to do in the back branches is use the patch I posted before,
to suppress the duplicate -l switches only on macOS.  In HEAD,
I propose we simplify life by doing it everywhere, as attached.

            regards, tom lane

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 18240b5fef..7b66590801 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -589,19 +589,27 @@ endif
 libpq = -L$(libpq_builddir) -lpq

 # libpq_pgport is for use by client executables (not libraries) that use libpq.
-# We force clients to pull symbols from the non-shared libraries libpgport
-# and libpgcommon rather than pulling some libpgport symbols from libpq just
-# because libpq uses those functions too.  This makes applications less
-# dependent on changes in libpq's usage of pgport (on platforms where we
-# don't have symbol export control for libpq).  To do this we link to
-# pgport before libpq.  This does cause duplicate -lpgport's to appear
-# on client link lines, since that also appears in $(LIBS).
+# We used to use this to force libpgport and libpgcommon to be linked before
+# libpq, ensuring that clients would pull symbols from those libraries rather
+# than possibly getting them from libpq (and thereby having an unwanted
+# dependency on which symbols libpq uses).  However, now that we can prevent
+# libpq from exporting those symbols on all platforms of interest, we don't
+# worry about that anymore.  The previous technique resulted in duplicate
+# libraries in link commands, since those libraries also appear in $(LIBS).
+# Some platforms warn about that, so avoiding those warnings is now more
+# important.  Hence, $(libpq_pgport) is now equivalent to $(libpq), but we
+# still recommend using it for client executables in case some other reason
+# appears to handle them differently.
+libpq_pgport = $(libpq)
+
 # libpq_pgport_shlib is the same idea, but for use in client shared libraries.
+# We need those clients to use the shlib variants.  (Ideally, users of this
+# macro would strip libpgport and libpgcommon from $(LIBS), but no harm is
+# done if they don't, since they will have satisfied all their references
+# from these libraries.)
 ifdef PGXS
-libpq_pgport = -L$(libdir) -lpgcommon -lpgport $(libpq)
 libpq_pgport_shlib = -L$(libdir) -lpgcommon_shlib -lpgport_shlib $(libpq)
 else
-libpq_pgport = -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/port -lpgport $(libpq)
 libpq_pgport_shlib = -L$(top_builddir)/src/common -lpgcommon_shlib -L$(top_builddir)/src/port -lpgport_shlib $(libpq)
 endif

Hi,

On 2023-09-29 12:14:40 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> On 2023-09-28 16:46:08 -0400, Tom Lane wrote:
> >>> Well, it's only important on platforms where we can't restrict
> >>> libpq.so from exporting all symbols.  I don't know how close we are
> >>> to deciding that such cases are no longer interesting to worry about.
> >>> Makefile.shlib seems to know how to do it everywhere except Windows,
> >>> and I imagine we know how to do it over in the MSVC scripts.
> 
> >> Hm, then I'd argue that we don't need to care about it anymore. The meson
> >> build does the necessary magic on windows, as do the current msvc scripts.
> 
> > If we take that argument seriously, then I'm inclined to adjust my
> > upthread patch for Makefile.global.in so that it removes the extra
> > inclusions of libpgport/libpgcommon everywhere, not only macOS.
> > The rationale would be that it's not worth worrying about ABI
> > stability details on any straggler platforms.
> 
> Looking closer, it's only since v16 that we have export list support
> on all officially-supported platforms.

Oh, right, before that Solaris didn't support it. I guess we could backpatch
that commit, it's simple enough, but I don't think I care enough about Solaris
to do so.


> Therefore, I think the prudent thing to do in the back branches is use the
> patch I posted before, to suppress the duplicate -l switches only on macOS.
> In HEAD, I propose we simplify life by doing it everywhere, as attached.

Makes sense.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-09-29 12:14:40 -0400, Tom Lane wrote:
>> Looking closer, it's only since v16 that we have export list support
>> on all officially-supported platforms.

> Oh, right, before that Solaris didn't support it. I guess we could backpatch
> that commit, it's simple enough, but I don't think I care enough about Solaris
> to do so.

HPUX would be an issue too, as we never did figure out how to do
export control on that.  However, I doubt it would be a great idea
to back-patch export control in minor releases, even if we had
the patch at hand.  That would be an ABI break of its own, although
it'd only affect clients that hadn't done things quite correctly.

>> Therefore, I think the prudent thing to do in the back branches is use the
>> patch I posted before, to suppress the duplicate -l switches only on macOS.
>> In HEAD, I propose we simplify life by doing it everywhere, as attached.

> Makes sense.

Done that way.  Were you going to push the -Wl,-exported_symbols_list
change?

            regards, tom lane

Hi,

On 2023-09-30 13:28:01 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-09-29 12:14:40 -0400, Tom Lane wrote:
> >> Looking closer, it's only since v16 that we have export list support
> >> on all officially-supported platforms.
> 
> > Oh, right, before that Solaris didn't support it. I guess we could backpatch
> > that commit, it's simple enough, but I don't think I care enough about Solaris
> > to do so.
> 
> HPUX would be an issue too, as we never did figure out how to do
> export control on that.

Ah, right.


> However, I doubt it would be a great idea
> to back-patch export control in minor releases, even if we had
> the patch at hand.  That would be an ABI break of its own, although
> it'd only affect clients that hadn't done things quite correctly.

Agreed.


> >> Therefore, I think the prudent thing to do in the back branches is use the
> >> patch I posted before, to suppress the duplicate -l switches only on macOS.
> >> In HEAD, I propose we simplify life by doing it everywhere, as attached.
> 
> > Makes sense.
> 
> Done that way.  Were you going to push the -Wl,-exported_symbols_list
> change?

Done now.

Greetings,

Andres Freund

Hi,

On 2023-09-29 11:11:49 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-09-28 22:53:09 -0400, Tom Lane wrote:
> >> (Perhaps we should apply the above to HEAD alongside the meson.build fix, to
> >> get more test coverage?)
> 
> > The macos animals BF seem to run Ventura, so I think it'd not really provide
> > additional coverage that CI and your manual testing already has. So probably
> > not worth it from that angle?
> 
> My thought was that if it's in the tree we'd get testing from
> non-buildfarm sources.

I'm not against it, but it also doesn't quite seem necessary, given your
testing on Catalina.


> FWIW, I'm going to update sifaka/indri to Sonoma before too much longer
> (they're already using Xcode 15.0 which is the Sonoma toolchain).
> I recently pulled longfin up to Ventura, and plan to leave it on that
> for the next year or so.  I don't think anyone else is running macOS
> animals.

It indeed looks like nobody is. I wonder if it's worth setting up a gcc macos
animal, it's not too hard to imagine that breaking.

Greetings,

Andres Freund

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2023-09-29 12:14:40 -0400, Tom Lane wrote:
>>> Therefore, I think the prudent thing to do in the back branches is use the
>>> patch I posted before, to suppress the duplicate -l switches only on macOS.
>>> In HEAD, I propose we simplify life by doing it everywhere, as attached.

>> Makes sense.

> Done that way.

So, in the no-good-deed-goes-unpunished department, I see that Noah's
AIX animals are now complaining about a few duplicate symbols, eg
while building initdb:

ld: 0711-224 WARNING: Duplicate symbol: .pg_encoding_to_char
ld: 0711-224 WARNING: Duplicate symbol: .pg_valid_server_encoding
ld: 0711-224 WARNING: Duplicate symbol: .pg_valid_server_encoding_id
ld: 0711-224 WARNING: Duplicate symbol: .pg_char_to_encoding

It's far from clear to me why we see this warning now when we didn't
before, because there are strictly fewer sources of these symbols in
the link than before.  They are available from libpgcommon and are
also intentionally exported from libpq.  Presumably, initdb is now
linking to these symbols from libpq where before it got them from the
first mention of libpgcommon, but why is that any more worthy of a
warning?

Anyway, I don't have a huge problem with just ignoring these warnings
as such, since AFAICT they're only showing up on AIX.

However, thinking about this made me realize that there's a related
problem.  At one time we had intentionally made initdb use its own
copy of these routines rather than letting it get them from libpq.
The reason is explained in commit 8468146b0:

    Fix the inadvertent libpq ABI breakage discovered by Martin Pitt: the
    renumbering of encoding IDs done between 8.2 and 8.3 turns out to break 8.2
    initdb and psql if they are run with an 8.3beta1 libpq.so.

This policy is still memorialized in a comment in initdb/Makefile:

# Note: it's important that we link to encnames.o from libpgcommon, not
# from libpq, else we have risks of version skew if we run with a libpq
# shared library from a different PG version.  The libpq_pgport macro
# should ensure that that happens.

and pg_wchar.h has this related comment:

 * XXX    We must avoid renumbering any backend encoding until libpq's major
 * version number is increased beyond 5; it turns out that the backend
 * encoding IDs are effectively part of libpq's ABI as far as 8.2 initdb and
 * psql are concerned.

Now it's not happening that way.  How big a problem is that?

In the case of psql, I think it's actually fixing a latent bug.
8468146b0's changes in psql clearly intend that psql will be
linking to libpq's copies of pg_char_to_encoding and
pg_valid_server_encoding_id, which is appropriate because it's
dealing with libpq's encoding IDs.  We broke that when we moved
encnames.c into libpgcommon.  We've not made any encoding ID
redefinitions since then, and we'd be unlikely to renumber PG_UTF8
in any case, but clearly linking to libpq's copies is safer.

(Which means that the makefiles are now OK, but the meson
build is not: we need libpq to be linked before libpgcommon.)

However, in the case of initdb, we had better be using the same
encoding IDs as the backend code we are setting up the database for.
If we ever add/renumber any backend-safe encodings again, we'd be
exposed to the same problem that 8.3 had.

Assuming that this problem is restricted to initdb, which I think
is true, probably the best fix is to cause the initdb link *only*
to link libpgcommon before libpq.  Every other non-backend program
is interested in libpq's encoding IDs if it cares at all.

Thoughts?

            regards, tom lane

I wrote:
> Assuming that this problem is restricted to initdb, which I think
> is true, probably the best fix is to cause the initdb link *only*
> to link libpgcommon before libpq.  Every other non-backend program
> is interested in libpq's encoding IDs if it cares at all.

The more I thought about that the less I liked it.  We're trying to
get away from link order dependencies, not add more.  And the fact
that we've had a latent bug for awhile from random-ish changes in
link order should reinforce our desire to get out of that business.

So I experimented with fixing things so that the versions of these
functions exported by libpq have physically different names from those
that you'd get from linking to libpgcommon.a or libpgcommon_srv.a.
Then, there's certainty about which one a given usage will link to,
based on what the #define environment is when the call is compiled.

This leads to a pleasingly small patch, at least in the Makefile
universe (I've not attempted to sync the meson or MSVC infrastructure
with this yet).  As a bonus, it should silence those new warnings
on AIX.  A disadvantage is that this causes an ABI break for
backend extensions, so we couldn't consider back-patching it.
But I think that's fine given that the problem is only latent
in released branches.

Thoughts?

            regards, tom lane


diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index d69bd89572..e80e57e457 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -16,13 +16,12 @@ subdir = src/bin/initdb
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global

-override CPPFLAGS := -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(ICU_CFLAGS) $(CPPFLAGS)
-
 # Note: it's important that we link to encnames.o from libpgcommon, not
 # from libpq, else we have risks of version skew if we run with a libpq
-# shared library from a different PG version.  The libpq_pgport macro
-# should ensure that that happens.
-#
+# shared library from a different PG version.  Define
+# USE_PRIVATE_ENCODING_FUNCS to ensure that that happens.
+override CPPFLAGS := -DUSE_PRIVATE_ENCODING_FUNCS -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(ICU_CFLAGS)
$(CPPFLAGS)
+
 # We need libpq only because fe_utils does.
 LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport) $(ICU_LIBS)

diff --git a/src/common/Makefile b/src/common/Makefile
index cc5c54dcee..70884be00c 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -140,6 +140,13 @@ libpgcommon.a: $(OBJS_FRONTEND)
     rm -f $@
     $(AR) $(AROPT) $@ $^

+#
+# Files in libpgcommon.a should use/export the "xxx_private" versions
+# of pg_char_to_encoding() and friends.
+#
+$(OBJS_FRONTEND): CPPFLAGS += -DUSE_PRIVATE_ENCODING_FUNCS
+
+
 #
 # Shared library versions of object files
 #
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index 25276b199f..7d2fad91e6 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -13,6 +13,8 @@
  *        included by libpq client programs.  In particular, a libpq client
  *        should not assume that the encoding IDs used by the version of libpq
  *        it's linked to match up with the IDs declared here.
+ *        To help prevent mistakes, relevant functions that are exported by
+ *        libpq have a physically different name when being referenced directly.
  *
  *-------------------------------------------------------------------------
  */
@@ -562,6 +564,23 @@ surrogate_pair_to_codepoint(pg_wchar first, pg_wchar second)
 }


+/*
+ * The functions in this list are exported by libpq, and we need to be sure
+ * that we know which calls are satisfied by libpq and which are satisfied
+ * by static linkage to libpgcommon.  (This is because we might be using a
+ * libpq.so that's of a different major version and has different encoding
+ * IDs from what libpgcommon knows.)  The official function names are what
+ * is actually used in and exported by libpq, while the names exported by
+ * libpgcommon.a and libpgcommon_srv.a end in "_private".
+ */
+#if defined(USE_PRIVATE_ENCODING_FUNCS) || !defined(FRONTEND)
+#define pg_char_to_encoding            pg_char_to_encoding_private
+#define pg_encoding_to_char            pg_encoding_to_char_private
+#define pg_valid_server_encoding    pg_valid_server_encoding_private
+#define pg_valid_server_encoding_id    pg_valid_server_encoding_id_private
+#define pg_utf_mblen                pg_utf_mblen_private
+#endif
+
 /*
  * These functions are considered part of libpq's exported API and
  * are also declared in libpq-fe.h.

I wrote:
> So I experimented with fixing things so that the versions of these
> functions exported by libpq have physically different names from those
> that you'd get from linking to libpgcommon.a or libpgcommon_srv.a.
> Then, there's certainty about which one a given usage will link to,
> based on what the #define environment is when the call is compiled.

> This leads to a pleasingly small patch, at least in the Makefile
> universe (I've not attempted to sync the meson or MSVC infrastructure
> with this yet).

Here's a v2 that addresses the meson infrastructure as well.
(This is my first attempt at writing meson code, so feel free
to critique.)  I propose not bothering to fix src/tools/msvc/,
on the grounds that (a) that infrastructure will likely be gone
before v17 ships, and (b) the latent problem with version
skew between libpq.dll and calling programs seems much less
likely to matter in the Windows world in the first place.

Barring comments or CI failures, I intend to push this soon.

            regards, tom lane

diff --git a/src/bin/initdb/Makefile b/src/bin/initdb/Makefile
index d69bd89572..e80e57e457 100644
--- a/src/bin/initdb/Makefile
+++ b/src/bin/initdb/Makefile
@@ -16,13 +16,12 @@ subdir = src/bin/initdb
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global

-override CPPFLAGS := -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(ICU_CFLAGS) $(CPPFLAGS)
-
 # Note: it's important that we link to encnames.o from libpgcommon, not
 # from libpq, else we have risks of version skew if we run with a libpq
-# shared library from a different PG version.  The libpq_pgport macro
-# should ensure that that happens.
-#
+# shared library from a different PG version.  Define
+# USE_PRIVATE_ENCODING_FUNCS to ensure that that happens.
+override CPPFLAGS := -DUSE_PRIVATE_ENCODING_FUNCS -I$(libpq_srcdir) -I$(top_srcdir)/src/timezone $(ICU_CFLAGS)
$(CPPFLAGS)
+
 # We need libpq only because fe_utils does.
 LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport) $(ICU_LIBS)

diff --git a/src/bin/initdb/meson.build b/src/bin/initdb/meson.build
index 49743630aa..28a2df99e4 100644
--- a/src/bin/initdb/meson.build
+++ b/src/bin/initdb/meson.build
@@ -7,19 +7,25 @@ initdb_sources = files(

 initdb_sources += timezone_localtime_source

-#fixme: reimplement libpq_pgport logic
-
 if host_system == 'windows'
   initdb_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
     '--NAME', 'initdb',
     '--FILEDESC', 'initdb - initialize a new database cluster',])
 endif

+# Note: it's important that we link to encnames.o from libpgcommon, not
+# from libpq, else we have risks of version skew if we run with a libpq
+# shared library from a different PG version.  Define
+# USE_PRIVATE_ENCODING_FUNCS to ensure that that happens.
+c_args = default_bin_args.get('c_args', []) + ['-DUSE_PRIVATE_ENCODING_FUNCS']
+
 initdb = executable('initdb',
   initdb_sources,
   include_directories: [timezone_inc],
   dependencies: [frontend_code, libpq, icu, icu_i18n],
-  kwargs: default_bin_args,
+  kwargs: default_bin_args + {
+    'c_args': c_args,
+  },
 )
 bin_targets += initdb

diff --git a/src/common/Makefile b/src/common/Makefile
index cc5c54dcee..70884be00c 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -140,6 +140,13 @@ libpgcommon.a: $(OBJS_FRONTEND)
     rm -f $@
     $(AR) $(AROPT) $@ $^

+#
+# Files in libpgcommon.a should use/export the "xxx_private" versions
+# of pg_char_to_encoding() and friends.
+#
+$(OBJS_FRONTEND): CPPFLAGS += -DUSE_PRIVATE_ENCODING_FUNCS
+
+
 #
 # Shared library versions of object files
 #
diff --git a/src/common/meson.build b/src/common/meson.build
index 3b97497d1a..ae05ac63cf 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -112,8 +112,8 @@ common_sources_frontend_static += files(
   'logging.c',
 )

-# Build pgport once for backend, once for use in frontend binaries, and once
-# for use in shared libraries
+# Build pgcommon once for backend, once for use in frontend binaries, and
+# once for use in shared libraries
 #
 # XXX: in most environments we could probably link_whole pgcommon_shlib
 # against pgcommon_static, instead of compiling twice.
@@ -131,6 +131,9 @@ pgcommon_variants = {
   '': default_lib_args + {
     'sources': common_sources_frontend_static,
     'dependencies': [frontend_common_code],
+    # Files in libpgcommon.a should use/export the "xxx_private" versions
+    # of pg_char_to_encoding() and friends.
+    'c_args': ['-DUSE_PRIVATE_ENCODING_FUNCS'],
   },
   '_shlib': default_lib_args + {
     'pic': true,
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index 25276b199f..29cd5732f1 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -13,6 +13,9 @@
  *        included by libpq client programs.  In particular, a libpq client
  *        should not assume that the encoding IDs used by the version of libpq
  *        it's linked to match up with the IDs declared here.
+ *        To help prevent mistakes, relevant functions that are exported by
+ *        libpq have a physically different name when being referenced
+ *        statically.
  *
  *-------------------------------------------------------------------------
  */
@@ -562,6 +565,23 @@ surrogate_pair_to_codepoint(pg_wchar first, pg_wchar second)
 }


+/*
+ * The functions in this list are exported by libpq, and we need to be sure
+ * that we know which calls are satisfied by libpq and which are satisfied
+ * by static linkage to libpgcommon.  (This is because we might be using a
+ * libpq.so that's of a different major version and has encoding IDs that
+ * differ from the current version's.)  The nominal function names are what
+ * are actually used in and exported by libpq, while the names exported by
+ * libpgcommon.a and libpgcommon_srv.a end in "_private".
+ */
+#if defined(USE_PRIVATE_ENCODING_FUNCS) || !defined(FRONTEND)
+#define pg_char_to_encoding            pg_char_to_encoding_private
+#define pg_encoding_to_char            pg_encoding_to_char_private
+#define pg_valid_server_encoding    pg_valid_server_encoding_private
+#define pg_valid_server_encoding_id    pg_valid_server_encoding_id_private
+#define pg_utf_mblen                pg_utf_mblen_private
+#endif
+
 /*
  * These functions are considered part of libpq's exported API and
  * are also declared in libpq-fe.h.

Hi,

On 2023-10-05 13:17:25 -0400, Tom Lane wrote:
> I wrote:
> > So I experimented with fixing things so that the versions of these
> > functions exported by libpq have physically different names from those
> > that you'd get from linking to libpgcommon.a or libpgcommon_srv.a.
> > Then, there's certainty about which one a given usage will link to,
> > based on what the #define environment is when the call is compiled.

I think that's a good plan. IIRC I previously complained about the symbols
existing in multiple places...  Don't remember the details, IIRCI I saw
warnings about symbol conflicts in extensions using libpq.


> > This leads to a pleasingly small patch, at least in the Makefile
> > universe (I've not attempted to sync the meson or MSVC infrastructure
> > with this yet).
> 
> Here's a v2 that addresses the meson infrastructure as well.
> (This is my first attempt at writing meson code, so feel free
> to critique.)  I propose not bothering to fix src/tools/msvc/,
> on the grounds that (a) that infrastructure will likely be gone
> before v17 ships, and (b) the latent problem with version
> skew between libpq.dll and calling programs seems much less
> likely to matter in the Windows world in the first place.

Makes sense.


> +# Note: it's important that we link to encnames.o from libpgcommon, not
> +# from libpq, else we have risks of version skew if we run with a libpq
> +# shared library from a different PG version.  Define
> +# USE_PRIVATE_ENCODING_FUNCS to ensure that that happens.
> +c_args = default_bin_args.get('c_args', []) + ['-DUSE_PRIVATE_ENCODING_FUNCS']
> +
>  initdb = executable('initdb',
>    initdb_sources,
>    include_directories: [timezone_inc],
>    dependencies: [frontend_code, libpq, icu, icu_i18n],
> -  kwargs: default_bin_args,
> +  kwargs: default_bin_args + {
> +    'c_args': c_args,
> +  },

I think you can just pass c_args directly to executable() here, I think adding
c_args to default_bin_args would be a bad idea.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> I think you can just pass c_args directly to executable() here, I think adding
> c_args to default_bin_args would be a bad idea.

Hm.  IIUC that would result in an error if someone did try to
put some c_args into default_bin_args, and I didn't think it would
be appropriate for this code to fail in such a case.  Still, I see
there are a bunch of other ways to inject globally-used compilation
flags, so maybe you're right that it'd never need to happen.

            regards, tom lane

Hi,

On 2023-10-05 13:37:38 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I think you can just pass c_args directly to executable() here, I think adding
> > c_args to default_bin_args would be a bad idea.
> 
> Hm.  IIUC that would result in an error if someone did try to
> put some c_args into default_bin_args, and I didn't think it would
> be appropriate for this code to fail in such a case.  Still, I see
> there are a bunch of other ways to inject globally-used compilation
> flags, so maybe you're right that it'd never need to happen.

I think the other ways of injecting c_args compose better (and indeed other
options are either injected globally, or via declare_dependency()).

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-10-05 13:37:38 -0400, Tom Lane wrote:
>> Hm.  IIUC that would result in an error if someone did try to
>> put some c_args into default_bin_args, and I didn't think it would
>> be appropriate for this code to fail in such a case.  Still, I see
>> there are a bunch of other ways to inject globally-used compilation
>> flags, so maybe you're right that it'd never need to happen.

> I think the other ways of injecting c_args compose better (and indeed other
> options are either injected globally, or via declare_dependency()).

Done that way.

            regards, tom lane

On Sat, Oct 7, 2023 at 12:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Done that way.

Is there still outstanding work on this thread? Because I'm just now
using a new MacBook (M2, Ventura 13.6.2) and I'm getting a lot of this
kind of thing in a meson build:

[2264/2287] Linking target src/interfaces/ecpg/test/sql/parser
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'
[2266/2287] Linking target src/interfaces/ecpg/test/sql/insupd
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'
[2273/2287] Linking target src/interfaces/ecpg/test/sql/quote
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'
[2278/2287] Linking target src/interfaces/ecpg/test/sql/show
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'
[2280/2287] Linking target src/interfaces/ecpg/test/sql/sqlda
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2023-11-20 14:14:00 -0500, Robert Haas wrote:
> On Sat, Oct 7, 2023 at 12:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Done that way.
> 
> Is there still outstanding work on this thread? Because I'm just now
> using a new MacBook (M2, Ventura 13.6.2) and I'm getting a lot of this
> kind of thing in a meson build:

Ventura? In that case I assume you installed newer developer tools? Because
otherwise I think we were talking about issues introduced in Sonoma.

Greetings,

Andres Freund

On Mon, Nov 20, 2023 at 2:35 PM Andres Freund <andres@anarazel.de> wrote:
> > Is there still outstanding work on this thread? Because I'm just now
> > using a new MacBook (M2, Ventura 13.6.2) and I'm getting a lot of this
> > kind of thing in a meson build:
>
> Ventura? In that case I assume you installed newer developer tools? Because
> otherwise I think we were talking about issues introduced in Sonoma.

I don't think I did anything special when installing developer tools.
xcode-select --version reports 2397, if that tells you anything.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2023-11-20 14:46:13 -0500, Robert Haas wrote:
> On Mon, Nov 20, 2023 at 2:35 PM Andres Freund <andres@anarazel.de> wrote:
> > > Is there still outstanding work on this thread? Because I'm just now
> > > using a new MacBook (M2, Ventura 13.6.2) and I'm getting a lot of this
> > > kind of thing in a meson build:
> >
> > Ventura? In that case I assume you installed newer developer tools? Because
> > otherwise I think we were talking about issues introduced in Sonoma.
>
> I don't think I did anything special when installing developer tools.
> xcode-select --version reports 2397, if that tells you anything.

Odd then. My m1-mini running Ventura, also reporting 2397, doesn't show any of
those warnings. I did a CI run with Sonoma, and that does show them.

I'm updating said m1-mini it to Sonoma now, but that will take until I have to
leave for an appointment.

Greetings,

Andres Freund

Robert Haas <robertmhaas@gmail.com> writes:
> Is there still outstanding work on this thread? Because I'm just now
> using a new MacBook (M2, Ventura 13.6.2) and I'm getting a lot of this
> kind of thing in a meson build:

13.6.2?  longfin's host is on 13.6.1, and the only thing Software
Update is showing me is an option to upgrade to Sonoma.  But anyway...

> [2264/2287] Linking target src/interfaces/ecpg/test/sql/parser
> ld: warning: -undefined error is deprecated
> ld: warning: ignoring duplicate libraries: '-lz'

Hmm ... I fixed these things in the autoconf build: neither my
buildfarm animals nor manual builds show any warnings.  I thought
the problems weren't there in the meson build.  Need to take another
look I guess.

            regards, tom lane

On Mon, Nov 20, 2023 at 3:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 13.6.2?  longfin's host is on 13.6.1, and the only thing Software
> Update is showing me is an option to upgrade to Sonoma.  But anyway...

Uh, I guess Apple made a special version just for me? That's
definitely what it says.

> > [2264/2287] Linking target src/interfaces/ecpg/test/sql/parser
> > ld: warning: -undefined error is deprecated
> > ld: warning: ignoring duplicate libraries: '-lz'
>
> Hmm ... I fixed these things in the autoconf build: neither my
> buildfarm animals nor manual builds show any warnings.  I thought
> the problems weren't there in the meson build.  Need to take another
> look I guess.

They're definitely there for me, and there are a whole lot of them. I
would have thought that if they were there for you in the meson build
you would have noticed, since ninja suppresses a lot of distracting
output that make prints.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Nov 20, 2023 at 3:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 13.6.2?  longfin's host is on 13.6.1, and the only thing Software
>> Update is showing me is an option to upgrade to Sonoma.  But anyway...

> Uh, I guess Apple made a special version just for me? That's
> definitely what it says.

Might be for M-series only; longfin's host is still Intel.

>> Hmm ... I fixed these things in the autoconf build: neither my
>> buildfarm animals nor manual builds show any warnings.  I thought
>> the problems weren't there in the meson build.  Need to take another
>> look I guess.

> They're definitely there for me, and there are a whole lot of them. I
> would have thought that if they were there for you in the meson build
> you would have noticed, since ninja suppresses a lot of distracting
> output that make prints.

I'm generally still using autoconf, I only run meson builds when
somebody complains about them ;-).  But yeah, I see lots of
"ld: warning: -undefined error is deprecated" when I do that.
This seems to have been installed by Andres' 9a95a510a:

   ldflags += ['-isysroot', pg_sysroot]
+  # meson defaults to -Wl,-undefined,dynamic_lookup for modules, which we
+  # don't want because a) it's different from what we do for autoconf, b) it
+  # causes warnings starting in macOS Ventura
+  ldflags_mod += ['-Wl,-undefined,error']

The autoconf side seems to just be letting this option default.
I'm not sure what the default choice is, but evidently it's not
"-undefined error"?  Or were they stupid enough to not allow you
to explicitly select the default behavior?

Also, I *don't* see any complaints about duplicate libraries.
What build options are you using?

            regards, tom lane

I wrote:
> The autoconf side seems to just be letting this option default.
> I'm not sure what the default choice is, but evidently it's not
> "-undefined error"?  Or were they stupid enough to not allow you
> to explicitly select the default behavior?

Seems we are not the only project having trouble with this:

https://github.com/mesonbuild/meson/issues/12450

I had not realized that Apple recently wrote themselves a whole
new linker, but apparently that's why all these deprecation
warnings are showing up.  It's not exactly clear whether
"deprecation" means they actually plan to remove the feature
later, or just that some bozo decided that explicitly specifying
the default behavior is bad style.

            regards, tom lane

Hi,

On 2023-11-20 16:20:20 -0500, Tom Lane wrote:
> I'm generally still using autoconf, I only run meson builds when
> somebody complains about them ;-).  But yeah, I see lots of
> "ld: warning: -undefined error is deprecated" when I do that.
> This seems to have been installed by Andres' 9a95a510a:
>
>    ldflags += ['-isysroot', pg_sysroot]
> +  # meson defaults to -Wl,-undefined,dynamic_lookup for modules, which we
> +  # don't want because a) it's different from what we do for autoconf, b) it
> +  # causes warnings starting in macOS Ventura
> +  ldflags_mod += ['-Wl,-undefined,error']

That's not the sole issue, because meson automatically adds that for binaries
(due to some linker issue that existed in the past IIRC).


> The autoconf side seems to just be letting this option default.
> I'm not sure what the default choice is, but evidently it's not
> "-undefined error"?  Or were they stupid enough to not allow you
> to explicitly select the default behavior?

I'm somewhat confused by what Apple did. I just was upgrading my m1 mac mini
to sonoma, and in one state I recall man ld documenting it below "Obsolete
Options". But then I also updated xcode, and now there's no mention whatsoever
of the option being deprecated in the man page at all. Perhaps my mind is
playing tricks on me.

And yes, it sure looks like everything other than 'dynamic_lookup' creates a
warning. Which seems absurd.


> Also, I *don't* see any complaints about duplicate libraries.
> What build options are you using?

I don't understand what Apple is thinking here. Getting the same library name
twice, e.g. icu once directly and once indirectly via xml2-config --libs or
such, seems very common. To avoid that portably, you basically need to do a
topographical sort of the libraries, to figure out an ordering that
deduplicates but doesn't move a library to before where it's used. With a
bunch of complexities due to -L, which could lead to finding different
libraries for the same library name, thrown in.


WRT Robert seeing those warnings and Tom not: There's something odd going
on. I couldn't immediately reproduce it. Then I realized it reproduces against
a homebrew install but not a macports one.

Robert, which are you using?

<a bunch later>

Meson actually *tries* to avoid this warning without resulting in incorrect
results due to ordering. But homebrew does something odd, with libxml-2.0,
zlib and a few others: Unless you do something to change that, brew has
/opt/homebrew/Library/Homebrew/os/mac/pkgconfig/14/ in its search path, but
those libraries aren't from homebrew, they're referencing macos
frameworks. Apparently meson isn't able to understand which files those .pc
files link to and gives up on the deduplicating.

If I add to the pkg-config search path, e.g. via
meson configure
-Dpkg_config_path=$OTHER_STUFF:/opt/homebrew/opt/zlib/lib/pkgconfig/:/opt/homebrew/opt/libxml2/lib/pkgconfig/

the warnings about duplicate libraries vanish.

Greetings,

Andres Freund

On Mon, Nov 20, 2023 at 11:37 PM Andres Freund <andres@anarazel.de> wrote:
> WRT Robert seeing those warnings and Tom not: There's something odd going
> on. I couldn't immediately reproduce it. Then I realized it reproduces against
> a homebrew install but not a macports one.
>
> Robert, which are you using?

macports

> Meson actually *tries* to avoid this warning without resulting in incorrect
> results due to ordering. But homebrew does something odd, with libxml-2.0,
> zlib and a few others: Unless you do something to change that, brew has
> /opt/homebrew/Library/Homebrew/os/mac/pkgconfig/14/ in its search path, but
> those libraries aren't from homebrew, they're referencing macos
> frameworks. Apparently meson isn't able to understand which files those .pc
> files link to and gives up on the deduplicating.
>
> If I add to the pkg-config search path, e.g. via
> meson configure
-Dpkg_config_path=$OTHER_STUFF:/opt/homebrew/opt/zlib/lib/pkgconfig/:/opt/homebrew/opt/libxml2/lib/pkgconfig/
>
> the warnings about duplicate libraries vanish.

Hmm, I'm happy to try something here, but I'm not sure exactly what.
I'm not setting pkg_config_path at all right now. I'm using this:

meson setup $HOME/pgsql $HOME/pgsql-meson -Dcassert=true -Ddebug=true
-Dextra_include_dirs=/opt/local/include
-Dextra_lib_dirs=/opt/local/lib -Dprefix=$HOME/install/dev

--
Robert Haas
EDB: http://www.enterprisedb.com

On 21.11.23 14:35, Robert Haas wrote:
> On Mon, Nov 20, 2023 at 11:37 PM Andres Freund <andres@anarazel.de> wrote:
>> WRT Robert seeing those warnings and Tom not: There's something odd going
>> on. I couldn't immediately reproduce it. Then I realized it reproduces against
>> a homebrew install but not a macports one.
>>
>> Robert, which are you using?
> 
> macports

Btw., I'm also seeing warnings like this.  I'm using homebrew.  Here is 
a sample:

[145/147] Linking target src/test/modules/test_shm_mq/test_shm_mq.dylib
-macosx_version_min has been renamed to -macos_version_min
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lgcc'
[146/147] Linking target src/test/modules/test_slru/test_slru.dylib

Hi,

> > On Mon, Nov 20, 2023 at 11:37 PM Andres Freund <andres@anarazel.de> wrote:
> >> WRT Robert seeing those warnings and Tom not: There's something odd going
> >> on. I couldn't immediately reproduce it. Then I realized it reproduces against
> >> a homebrew install but not a macports one.
> >>
> >> Robert, which are you using?
> >
> > macports
>
> Btw., I'm also seeing warnings like this.  I'm using homebrew.  Here is
> a sample:
>
> [145/147] Linking target src/test/modules/test_shm_mq/test_shm_mq.dylib
> -macosx_version_min has been renamed to -macos_version_min
> ld: warning: -undefined error is deprecated
> ld: warning: ignoring duplicate libraries: '-lgcc'
> [146/147] Linking target src/test/modules/test_slru/test_slru.dylib

I prefer not to build Postgres on Mac because it slows down GMail and
Slack. After reading this discussion I tried and I can confirm I see
the same warnings Robert does:

```
[322/1905] Linking target src/interfaces/libpq/libpq.5.dylib
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'
[326/1905] Linking target src/timezone/zic
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lz'
[1113/1905] Linking target src/backend/postgres
ld: warning: -undefined error is deprecated
ld: warning: ignoring duplicate libraries: '-lpam', '-lxml2', '-lz'
[1124/1905] Linking target src/backend/replication/pgoutput/pgoutput.dylib
ld: warning: -undefined error is deprecated
[1125/1905] Linking target
src/backend/replication/libpqwalreceiver/libpqwalreceiver.dylib
ld: warning: -undefined error is deprecated

... many more ...
```

My laptop is an Intel MacBook Pro 2019. The MacOS version is Sonoma
14.0 and I'm using homebrew. `xcode-select --version` says 2399. I'm
using the following command:

```
XML_CATALOG_FILES=/usr/local/etc/xml/catalog time -p sh -c 'git clean
-dfx && meson setup --buildtype debug -DPG_TEST_EXTRA="kerberos ldap
ssl" -Dldap=disabled -Dssl=openssl -Dcassert=true -Dtap_tests=enabled
-Dprefix=/Users/eax/pginstall build && ninja -C build && ninja -C
build docs && meson test -C build'
```

I don't see any warnings when using Autotools.

--
Best regards,
Aleksander Alekseev

On Tue, Nov 21, 2023 at 9:59 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> Btw., I'm also seeing warnings like this.  I'm using homebrew.  Here is
> a sample:
>
> [145/147] Linking target src/test/modules/test_shm_mq/test_shm_mq.dylib
> -macosx_version_min has been renamed to -macos_version_min
> ld: warning: -undefined error is deprecated
> ld: warning: ignoring duplicate libraries: '-lgcc'
> [146/147] Linking target src/test/modules/test_slru/test_slru.dylib

I poked at this issue a bit more. In meson.build, for Darwin, we have this:

  # meson defaults to -Wl,-undefined,dynamic_lookup for modules, which we
  # don't want because a) it's different from what we do for autoconf, b) it
  # causes warnings starting in macOS Ventura
  ldflags_mod += ['-Wl,-undefined,error']

I don't really understand how meson works, but I blindly tried
commenting that out. What I found is that doing so reduces the number
of warnings that I get locally from 226 to 113. The difference seems
to be that, with the unpatched meson.build file, I get warnings both
about binaries and also about loadable modules, but with the patched
version, the loadable modules stop emitting warnings, and the binaries
continue to do so. This gives the flavor:

-[] Linking target contrib/adminpack/adminpack.dylib
-[] Linking target contrib/amcheck/amcheck.dylib
...
-[] Linking target src/backend...version_procs/latin2_and_win1250.dylib
-[] Linking target src/backend...version_procs/utf8_and_iso8859_1.dylib
 [] Linking target src/backend/postgres
-[] Linking target src/backend/replication/pgoutput/pgoutput.dylib
-[] Linking target src/backend/snowball/dict_snowball.dylib
 [] Linking target src/bin/initdb/initdb
 [] Linking target src/bin/pg_amcheck/pg_amcheck
 [] Linking target src/bin/pg_archivecleanup/pg_archivecleanup
 [] Linking target src/bin/pg_basebackup/pg_basebackup
...

The lines with - at the beginning are the warnings that disappear when
I comment out the addition to ldflags_mod. The lines without a - at
the beginning are the ones that appear either way.

The first, rather inescapable, conclusion is that the comment isn't
completely correct. It claims that we need to add -Wl,-undefined,error
on macOS Ventura to avoid warnings, but on my system it has exactly
the opposite effect: it produces warnings. I think we must have
misdiagnosed what the triggering condition actually is -- maybe it
depends on CPU architecture or choice of compiler or something, but
it's not as simple as "on Ventura you need this" because I am on
Ventura and I anti-need this.

The second conclusion that I draw is that there's something in meson
itself which is adding -Wl,-undefined,error when building binaries.
The snippet above is the only place in the entire source tree where we
specify a -undefined flag for a compile. The fact that the warning
still shows up when I comment that out means that in other cases,
meson itself is adding the flag, seemingly wrongly. But I don't know
how to make it not do that. I tried adding an option to ldflags, but
the linker isn't happy with adding something like
-Wl,-undefined,warning --- then it complains about both
-Wl,-undefined,error and -Wl,-undefined,warning. Apparently, what it
really wants is for the option to not be specified at all:

https://stackoverflow.com/questions/77525544/apple-linker-warning-ld-warning-undefined-error-is-deprecated

See also https://github.com/mesonbuild/meson/issues/12450

What a stupid, annoying decision on Apple's part. It seems like
-Wl,-undefined,error is the default behavior, so they could have just
ignored that flag if present, but instead they complain about being
asked to do what they were going to do anyway.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2023-11-28 10:48:04 -0500, Robert Haas wrote:
> The second conclusion that I draw is that there's something in meson
> itself which is adding -Wl,-undefined,error when building binaries.

Right.


> What a stupid, annoying decision on Apple's part. It seems like
> -Wl,-undefined,error is the default behavior, so they could have just
> ignored that flag if present, but instead they complain about being
> asked to do what they were going to do anyway.

Especially because I think it didn't actually use to be the default when
building a dylib.


While not helpful for this, I just noticed that there is
-no_warn_duplicate_libraries, which would at least get rid of the
  ld: warning: ignoring duplicate libraries: '-lxml2'
style warnings.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-11-28 10:48:04 -0500, Robert Haas wrote:
>> What a stupid, annoying decision on Apple's part. It seems like
>> -Wl,-undefined,error is the default behavior, so they could have just
>> ignored that flag if present, but instead they complain about being
>> asked to do what they were going to do anyway.

> Especially because I think it didn't actually use to be the default when
> building a dylib.

Indeed.  Whoever's in charge of their linker now seems to be quite
clueless, or at least aggressively anti-backwards-compatibility.

> While not helpful for this, I just noticed that there is
> -no_warn_duplicate_libraries, which would at least get rid of the
>   ld: warning: ignoring duplicate libraries: '-lxml2'
> style warnings.

Oh!  If that's been there long enough to rely on, that does seem
very helpful.

            regards, tom lane

Hi,

On 2023-11-30 21:24:21 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-11-28 10:48:04 -0500, Robert Haas wrote:
> >> What a stupid, annoying decision on Apple's part. It seems like
> >> -Wl,-undefined,error is the default behavior, so they could have just
> >> ignored that flag if present, but instead they complain about being
> >> asked to do what they were going to do anyway.
> 
> > Especially because I think it didn't actually use to be the default when
> > building a dylib.
> 
> Indeed.  Whoever's in charge of their linker now seems to be quite
> clueless, or at least aggressively anti-backwards-compatibility.

It looks like it even affects a bunch of Apple's own products [1]...


> > While not helpful for this, I just noticed that there is
> > -no_warn_duplicate_libraries, which would at least get rid of the
> >   ld: warning: ignoring duplicate libraries: '-lxml2'
> > style warnings.
> 
> Oh!  If that's been there long enough to rely on, that does seem
> very helpful.

I think it's new too. But we can just check if the flag is supported.


This is really ridiculous. For at least part of venturas life they warned
about -Wl,-undefined,dynamic_lookup, which lead to 9a95a510adf3. They don't
seem to do that anymore, but now you can't specify -Wl,-undefined,error
anymore without a warning.


Attached is a prototype testing this via CI on both Sonoma and Ventura.


It's certainly possible to encounter the duplicate library issue with
autoconf, but probably not that common. So I'm not sure if we should inject
-Wl,-no_warn_duplicate_libraries as well?

Greetings,

Andres Freund


[1] https://developer.apple.com/forums/thread/733317

Hi,

Looks like I forgot to update the thread to note that I finally committed the
remaining warning fixes. I had already fixed a bunch of others in upstream
meson.

commit a3da95deee38ee067b0bead639c830eacbe894d5
Author: Andres Freund <andres@anarazel.de>
Date:   2024-03-13 01:40:53 -0700

    meson: macos: Avoid warnings on Sonoma

Greetings,

Andres Freund