Fix a linker bug with Clang on Windows

[mstembera]

When compiling
make build ARCH=x86-64-bmi2 COMP=clang -j
with Clang version 10.0.1 (latest from MSYS2) on Windows the link step fails with:

benchmark.o: file not recognized: file format not recognized
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [Makefile:844: stockfish] Error 1
make[1]: Leaving directory '/c/Projects/forks/Stockfish/src'
make: *** [Makefile:716: build] Error 2

bench: 3736029

[vondele]

that's a workaround for a compiler bug, right?
@gvreuls can you test this as well?

[mstembera]

Yes.

[vondele]

BTW, does this influence compilation on macOS?

[ArminHHJ]

I do a compile on macOS (10.16.6) yesterday and some minutes bevor.
make build ARCH=x86-64-bmi2 COMP=clang

No problems, binary running :)

[mstembera]

@vondele Thanks. It probably would have affected maOS. New version pushed.
I also wonder about lines 606 and 608(in master). Seems like they should be nested the same way I nested them in the patch.

[gvreuls]

@mstembera

I also wonder about lines 606 and 608(in master). Seems like they should be nested the same way I nested them in the patch.

https://github.com/official-stockfish/Stockfish/blob/e0bafa1911ede61b9268e0b461a5d8856d6cd6be/src/Makefile#L606
Means "if the string MINGW is found in $(KERNEL) then ..." (literally if the result of searching the string MINGW in $(KERNEL) is not empty then ...)

So basically lines 606 and 608 are an or condition
if "MINGW" or "MSYS" is found in $(KERNEL) then add -save-temps to CXXFLAGS

What you do is an and condition:
if "MINGW" and "MSYS" are found in $(KERNEL) then add -flto=thin to CXXFLAGS

Because $(KERNEL) either contains "MINGW" (when run from the MSYS shell) or "MSYS" (when run from the Windows CMD shell) but never both, your condition can never be true so this basically never adds -flto=thin to CXXFLAGS.

If you want to add -flto=thin to CXXFLAGS only if $(KERNEL) neither contains "MINGW" nor "MSYS" you do:

        ifeq ($(findstring MINGW,$(KERNEL)),)
        ifeq ($(findstring MSYS,$(KERNEL)),)
            CXXFLAGS += -flto=thin
        endif
        endif

(notice the change from ifneq to ifeq)

Edit: about the indentation: The convention is that if several ifeq/ifneq are part of the same condition but can't be written on a single line (because make doesn't have real && and || operators) you don't indent.

[snicolet]

Maybe we should not use link time optimization at all: we keep patching SF source and Makefile to workaround around buggy lto compiler implementations :-(

[vondele]

AFAIK, most issues have been windows related. So fixing #2958 would help.

[mstembera]

@gvreuls Thank you for taking the time to explain. Fixed.

[erbsenzaehler]

Using the llvm linker with adding the line -fuse-ld=lld to the linker parameters does work for me on msys2.

[mstembera]

@erbsenzaehler Using CXXFLAGS += -flto=thin -fuse-ld=lld I get this error:
clang++: error: invalid linker name in argument '-fuse-ld=lld'
Do I need something else to invoke the llvm linker?

[erbsenzaehler]

The linker is not installed by default. You can get it with pacman -Syu mingw-w64-x86_64-lld

[mstembera]

@erbsenzaehler Thanks! I can confirm that works. I leave it to @vondele whether we want to depend on the lld linker being installed or disable lto.

[vondele]

I would think that since clang on mingw is anyway not the default, we can expect users of clang to install the needed linker for a functional toolchain. Not so different from the situation on ubuntu where just installing clang++ is not enough needs llvm installed as well. So, I would tend to add the correct lld flag instead of disabling lto. @mstembera can you do that?

[mstembera]

Done. However should it be added for Windows only?

[mstembera]

Ok Appveyor didn't like the flag so windows only.