cmake: Drop Libtool's option

[hebasto]

The -no-undefined is not a linker flag, rather a Libtool flag, which we do not care about.

[hebasto]

Thanks to @m3dwards who reported this bug offline.

Friendly ping @TheCharlatan :)

[theuni]

Mmm, we still want this behavior for shared libs though.

A quick google says we want: string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,--no-undefined")

Or perhaps we could/should do it per-target.

[hebasto]

Mmm, we still want this behavior for shared libs though.

A quick google says we want: string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,--no-undefined")

Or perhaps we could/should do it per-target.

I was thinking something very similar while working on this PR.

Indeed, it might seem that Libtool's -no-undefined option is converted to the linker's -Wl,--no-undefined. However, that is not the case on the master branch. So I wonder, do we want to change behaviour at this point by adding a new linker option?

[theuni]

Hmm, I'm seeing the same. Not sure why. It definitely worked at some point in the past.

Yes, I think we should be using the flag when possible. I'll investigate why libtool isn't adding it on master.

[hebasto]

Indeed, it might seem that Libtool's -no-undefined option is converted to the linker's -Wl,--no-undefined. However, that is not the case on the master branch.

Same here: bitcoin-core/secp256k1#1556.

[real-or-random]

Indeed, it might seem that Libtool's -no-undefined option is converted to the linker's -Wl,--no-undefined. However, that is not the case on the master branch.

I checked the libtool and AFAIU -no-undefined has always been a no-op on Linux with GCC. I think -no-undefined was introduced in libtool to enable building shared libraries on some platforms where you need to pass a corresponding parameter to the linker (e.g., --Wl,no-undefined on Windows/GCC). libtool can't pass the linker parameter always because this would, e.g., exclude building static libraries with undefined symbols. That's why they needed this -no-undefined hack to let the developer declare explicitly that there won't be any undefined symbols. So the primary goal of -no-undefined was to make shared library builds work on these platforms, and not to err on the side of caution and make the linker verify that there are no undefined symbols.

Still, people have asked for this on Linux, but it was apparently never implemented. Here's some (Bitcoin-)prehistoric conversation about this: https://gcc.gnu.org/legacy-ml/gcc-patches/2001-02/msg00305.html. See in particular this one: https://gcc.gnu.org/legacy-ml/gcc-patches/2001-02/msg00376.html

I do think it is a reasonably important issue, because some platforms
require no undefined symbols when building shared libraries, but lots
of development is done on GNU/Linux, and so if `--no-undefined' doesn't
work on GNU/Linux, then shared libraries on AIX/Windows/Beos will
continually be broken by developers on GNU/Linux,

Agreed! That's precisely our issue here...

---

So I wonder, do we want to change behaviour at this point by adding a new linker option?

I think it's desirable to have the flag. The referenced thread mentions some issues when not linking to ld-linux explicitly, but I can't find recent mentions of this (this is >23 years ago). Gentoo also recommends it: https://wiki.gentoo.org/wiki/Project:Quality_Assurance/-Wl,-z,defs_and_-Wl,--no-allow-shlib-undefined If it really breaks something, we could still revert.

edit: But I don't know what the general strategy of the CMake migration is. If the plan is to switch to CMake first while trying to touch the actual build as little as possible, then the change should be postponed.

[hebasto]

@theuni

Yes, I think we should be using the flag when possible.

I do agree with you. But I cannot see any compelling reasons to mix it with the CMake migration. Shouldn't a new linker option be introduced in the master branch?

[theuni]

Thanks for the deep-dive @real-or-random!

libtool can't pass the linker parameter always because this would, e.g., exclude building static libraries with undefined symbols.

The linker/ldflags aren't used for static libs, rather ar/arflags :)

Still, I'm sure this has been used/needed at some point in the past. Is it possible it's wired up for gcc+mingw?

[hebasto]

Still, I'm sure this has been used/needed at some point in the past. Is it possible it's wired up for gcc+mingw?

The Libtool's -no-undefined option is a Libtool's requirement to build a DLL. However, the linker options are not affected.

[theuni]

Still, I'm sure this has been used/needed at some point in the past. Is it possible it's wired up for gcc+mingw?

The Libtool's -no-undefined option is a Libtool's requirement to build a DLL. However, the linker options are not affected.

Ok, maybe that's it then. I guess I just assumed it did the obvious thing (adding the linker flag). But that would explain why I remembered it doing "something".

[theuni]

utACK 414bdee.

I think we want this behavior, but as it's not present in master, we should start there.

[real-or-random]

I think we want this behavior, but as it's not present in master, we should start there.

What about creating a milestone here for things you want to do after CMake has been merged into master?

[fanquake]

Unless it's something drastic, I'd rather not delay things, where possible, until after CMake, because that's just going to widen the gap in functionality between the builds. If it's just the case of adding a new linker flag to the build, we can just do that now.

[m3dwards]

ACK 414bdee

Without this PR I would get:

ld: unknown options: -no-undefined
clang: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[3]: *** [src/kernel/CMakeFiles/bitcoinkernel.dir/build.make:1241: src/kernel/libbitcoinkernel.dylib] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1867: src/kernel/CMakeFiles/bitcoinkernel.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1651: src/CMakeFiles/bitcoin-chainstate.dir/rule] Error 2
gmake: *** [Makefile:803: bitcoin-chainstate] Error 2

[hebasto]

Unless it's something drastic, I'd rather not delay things, where possible, until after CMake, because that's just going to widen the gap in functionality between the builds.

There is no gap in functionality between the builds with this PR, unless I missed something.

If it's just the case of adding a new linker flag to the build, we can just do that now.

Yes, we can. FWIW, I opened a related PR in the secp256k1 repo.

[theuni]

utACK 414bdee