Don't override linker

[jpalus]

No description provided.

[faho]

What's the reason for this?

We want gold or lld because that avoids spurious errors about sys_nerr that we don't want to have reported and are faster. Is there a particular reason to not want them?

[jpalus]

In general Linux distributions want to have control over toolchain used unless package requires specific tools. I wouldn't even notice this preference if our 32bit builds didn't fail with lld. That's likely an issue with lld linker itself but this does not change the fact that we (PLD Linux) prefer to build packages with as consistent toolchain as possible. Apparently so does Gentoo.

[faho]

Okay, in that case make it one option, "FISH_USE_DEFAULT_LINKER=ON|OFF" that just skips the override. Let it default to OFF and document it in the README, together with a note that GNU's default prints spurious errors.

Or if that doesn't work for you make it "FISH_USE_LINKER=/path/to/linker", and if it's unset (by default) still do the override.

Having two options for this is awkward and unnecessary (among other things, they can be set at the same time - in the current implementation LLD wins, but that's an arbitrary choice and unclear from the outside).

(The exact names are, of course, up for debate)

Apparently so does Gentoo.

(Link is broken, I'm assuming you mean https://bugs.gentoo.org/790596)

[jpalus]

Sorry about the link, no idea where that '3' at the end came from.

Anyway before I make any changes I would like to clarify few things:

1. Warning about sys_nerr/sys_errlist comes from glibc versions 2.31 and older, it's no longer there in 2.32 and up
2. These warnings are implemented with .gnu.warning ELF section which both bfd and gold support so I assume gold preference is not there for handling those warnings, just for performance (comment in CMakeLists.txt is incorrect):

$ cat test.c       
int test() { return 0; }
static const char test_warning[] __attribute__((section(".gnu.warning.test"))) = "test link warning";

$ cat main.c
int test();
int main() { return test(); }

$ gcc -fuse-ld=bfd test.c main.c -o main
/usr/bin/ld.bfd: /home/users/jan/tmp/cclYUYyG.o: in function `main':
main.c:(.text+0x8): warning: test link warning

$ gcc -fuse-ld=gold test.c main.c -o main
/home/users/jan/tmp/ccJKFCEE.o:main.c:function main: warning: test link warning

3. lld does not issue warning simply because it lacks support for .gnu.warning. Adding such support was requested however it stalled. It is possible that lld will issue such warning one day though.

Can you confirm that you do want to keep this override for merely a warning that occurs with 1.5y old glibc and happens to be "fixed" by lld which lacks support for those warnings?

[faho]

"1.5y old glibc" means it's still very much in active use. That's newer than what you would expect in stable distros like Debian or RHEL or Ubuntu LTS.

Yes, we will keep workarounds for that.

[faho]

Okay, after a quick shower to moisturize those brain cells: It's about a warning, not functionality. In an old and crufty glibc version, only when you build fish yourself (instead of installing via our PPAs). That's very much unimportant cosmetics in a minority usecase.

So this entire mechanism can, actually, be nuked from orbit. If your default linker is slow, your default linker is slow, and you should do something about it.

[zanchey]

We got a handful of problem reports, which is why I think @ridiculousfish added it in 5f6ee7f, but I agree it can go.

[faho]

Okay, it has been removed.

[mqudsi]

I disagree with this, because a) there's really no standard way to set the "default" linker, and b) CMake makes it hard to override the linker even via the usual ways. Although this is a C++ project, if both CFLAGS and CPPFLAGS are set to -fuse-ld=lld, CMake still insists on invoking $CC separately and using the default gcc linker unless you also define CXXFLAGS to -fuse-ld=lld. I don't think the distinction between CFLAGS/CPPFLAGS/CXXFLAGS extends to the linker, technically using only CPPFLAGS here should suffice.

Also, @ridiculousfish added that hack for good reason: the warnings really did cause a lot of unnecessary drama (100% glibc's fault) and sent a lot of people barking up the wrong tree and absolutely deserve to be suffocated into silence and submission, by fire if necessary. Many (most?) LTS distros are still using glibc versions that stupidly complain with ZERO way of explicitly ignoring/opting-out of warnings, and it'll be ages before glibc's backtracking here will trickle down to everyone. (And the performance benefits are an extremely nice icing on the cake to boot.)

[mqudsi]

Ok, this is just stupid. The only way to get CMake to respect my preference for ld.lld is, as shown above, to add -fuse-ld=lld to CXXFLAGS. But if your default compiler (which, unlike the linker, can be set via standard means like $CC) is clang, you'll get a new warning for each and every source file compiled!

[34/117] Building CXX object CMakeFiles/fishlib.dir/src/builtin_cd.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[35/117] Building CXX object CMakeFiles/fishlib.dir/src/fish_version.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[36/117] Building CXX object CMakeFiles/fishlib.dir/src/builtin_history.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[37/117] Building CXX object CMakeFiles/fishlib.dir/src/builtin_functions.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[38/117] Building CXX object CMakeFiles/fishlib.dir/src/builtin.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[39/117] Building CXX object CMakeFiles/fishlib.dir/src/builtin_ulimit.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[40/117] Building CXX object CMakeFiles/fishlib.dir/src/builtin_complete.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
[41/117] Building CXX object CMakeFiles/fishlib.dir/src/color.cpp.o
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-fuse-ld=lld' [-Wunused-command-line-argument]

because CMake passes CXXFLAGS to each invocation of clang, including those that have nothing to do with linking. This is not OK.

[jpalus]

Enforcing specific toolchain is valid in very rare cases and fish is just not one of them.

Enforcing linker to fix somebody's warning (cosmetic thing) while breaking builds for others does not bring any added value.

If you have problem with new change then open new issue, don't reopen pull requests.

No idea why you pass linker flag in C++ compiler flags... try passing it in linker flags (LDFLAGS)?

[mqudsi]

LDFLAGS is passed to the linker, it doesn't determine what the linker is. In this case, the top-level "linker" happens to be the compiler that invokes the actual linker, so it does work, but that is not a guarantee. Hence my point about there not being a standard way to actually decide what linker to use. You're right that using it would avoid the warnings about unused arguments generated by clang, however.

I re-opened the pull request because it was my intention to merge your existing PR, but giving a window of opportunity to hear any other viewpoints.

[faho]

I re-opened the pull request because it was my intention to merge your existing PR, but giving a window of opportunity to hear any other viewpoints.

@mqudsi Please do handle the existing feedback: Having two options that aren't orthogonal is awkward. This should be one option. It should also be documented in README.md.

Other than that, I don't really care either way. As long as it's usable to all involved, go ahead.

[mqudsi]

That sounds good. I'll close this one out, then.

[jpalus]

the top-level "linker" happens to be the compiler that invokes the actual linker, so it does work, but that is not a guarantee

It works exactly how it supposed to. You as a user control what linker should be and what flags should be passed. CMake defaults to C++ compiler for linking stage so if not specified otherwise LDFLAGS are passed to C++ compiler. You do realize code that was dropped did exactly that? It used to add a linker flag. It does not influence what linker binary really is (it is still C++ compiler). So passing LDFLAGS is effectively equivalent to dropped logic.

[zanchey]

@mqudsi does https://stackoverflow.com/a/55801096/125549 (using cmake -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld) work for you?

[mqudsi]

@zanchey I think that's what setting LDFLAGS indirectly does. After some time to think about this and in particular given that it seems to be deterministic that CMake invokes $CXX (or whatever that resolved to) to do the linking, I guess it's fair enough for the dev to just set an LDFLAGS (or CMAKE_EXE_LINKER_FLAGS) that will trigger the desired linker behavior. We can always put the linker preference patch back in if we start getting downstream complaints or concerns about the warnings again.

[neheb]

To add to this, gold is actually broken under mips64.

There's very little benefit in using a faster linker. Empirical testing shows an increase in binary size.