LGTM

LGTM

LGTM

Feels a bit like approving myself (which is only partially true) but I don't see any issues with the patch itself.

Ah, so that explains why everyone sends LGTM text.
I'll try to remember it next time.

Wow, I haven't expected that.

Feels like a heavy hammer to me but guess there is no better option.

FWIW at MSYS2 we build LLVM with both GNU and LLVM toolchains for different "subsystems".
GNU's LLD was deadlocking a lot so we have patched it to use TerminateProcess and it worked around all deadlocks except when running tests (the patch probably needs more work on it), we don't apply it for LLVM's LLD because I thought it's a bit overkill.

Funny but I've hit this deadlock (LLVM_LINK_LLVM_DYLIB, LLD_DEFAULT_LD_LLD_IS_MINGW and shared libc++) last week in single LLD unit test. It never reproduced despite dozen of LLD testsuite runs.

When building with ninja, the output is buffered so the warnings appear directly after the specific build target that cause them.

But I don't know if ld.bfd actually implements the -z options for PE targets?

Fix typo and the test

I don't know when they exactly appear but look at my building log: https://reviews.llvm.org/P8263
The build was done with 16 threads so it's hard to tell which objects made it appear.

Sorry I thought it's simple enough on it's own.

The change itself looks correct but cannot speak for the test.

If compiling with GCC or linking with ld.bfd, these options have little

effect

I saw https://github.com/msys2/msys2-runtime/issues/36 but I have never worked with MSYS2 runtime internals so you probably know more than I do at this point.
https://github.com/dscho who maintains Git for Windows might have some insights.

It's clear overlook from D79995, that part is perfectly reasonable to me.

Sorry for the delay.
LGTM from MinGW point of view.

Opened https://reviews.llvm.org/D89225

Or maybe even make it into a cmake option?

Older versions of lld didn't error out if exceeding the limit, this is fixed in D86701, unfortunately after the 11 branch.

With -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_RANLIB_COMPILER=llvm-ranlib -DCMAKE_AR_COMPILER=llvm-ar -DCMAKE_LINKER=ld.lld -DLLVM_ENABLE_LLD=ON libclang-cpp.dll has 65535 exports, seems it got truncated?

Reproduced by adding -DLLVM_ENABLE_ASSERTIONS=ON.

How many exports does your DLL end up with - is it close to the 64k limit, or far from it? Is there maybe some other library that is linked in, that increases the number of exported symbols (=all), or does this DLL only export explicitly intended symbols with dllexport?

@ASDenysPetrov what CMake options do you use?
I'm unable to reproduce this error with Clang+LLD and GCC+Binutils from MSYS2.

I wonder if it'd be possible to add some custom substitution in the lit config, to have it replace ld.lld with ld.lld -m <some-elf> in this case...

With BFD (tested version 2.35) the latter specified option takes precedence:

Nit: according to https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem?view=vs-2019 the minimal and the default subsystem for ARM should be 6.02.

@hans sorry for not spotting it earlier but is it possible to still backport it to LLVM 11?
It's totally OK if it's too late. I can easily keep it as a patch in MSYS2 repository.

LGTM
Confirmed that GCC 9 still adds .exe.

Sounds great, could you commit it? I don't have the permission.

Cygwin seems to be the only Windows target that adds version to the library names.

Thanks.

Sorry, pasted wrong command. I was curious if import library has it (since static one did not) but in the end neither import nor static library shows any export.

what code appends a version number in dynamic libs when built for other platforms, but decides not to, when targeting windows in general, or mingw specifically?

bin/llvm-readobj --coff-directives lib/liblibclang.dll.a | grep -i export shows nothing.

Could you fix the title+description?

We had this patch at MSYS2 for years and I'm not aware of any issues with the static library.
I think the library looks fine:

$ nm lib/liblibclang.a | grep __imp_
                 U __imp___acrt_iob_func
                 U __imp___acrt_iob_func
                 U __imp_GetModuleFileNameA
                 U __imp_VirtualQuery
                 U __imp___acrt_iob_func
                 U __imp___acrt_iob_func
                 U __imp___acrt_iob_func

I don't use MSVC but apparently there is no libLLVM there: https://github.com/llvm/llvm-project/blob/9c651c231f3144f53e13cd0a1747589e1b2edccd/llvm/tools/llvm-shlib/CMakeLists.txt#L15

-DLLVM_ENABLE_PROJECTS="clang;lldb" -DLLVM_BUILD_LLVM_DYLIB=1 with only this patch applied (< is before, > is after):

$ diff <(cd ../build && find ./ | sort) <(cd ../build2/ && find ./ | sort)
38a39,41
> ./bin/libLLVM.dll
> ./bin/libLTO.dll
> ./bin/libRemarks.dll
51d53
< ./bin/LLVM.dll
119d120
< ./bin/LTO.dll
123d123
< ./bin/Remarks.dll
1460a1461
> ./lib/libclang.dll.a
1500,1501c1501
< ./lib/liblibclang.dll.a
< ./lib/libliblldb.dll.a
---
> ./lib/liblldb.dll.a

I haven't tried BUILD_SHARED_LIBS=TRUE TBH.

Also adjusted llvm-config.

Updated the diff.

How widespread are these build systems that parse help output? (Given that it took until now to discover them, I'd venture "not very".)

Thanks for the explanation.

FYI this doesn't fully fix MinGW issue:

$ cat tools/llvm-config/BuildVariables.inc | grep LLVM_SYSTEM_LIBS
#define LLVM_SYSTEM_LIBS "-lpsapi -lshell32 -lole32 -luuid -ladvapi32 -lz.dll"

It's because libraries are found by their import library which is libz.dll.a in this case.
libz.dll.a matches if in line 213 and gets stripped to z.dll which doesn't match shared library regex lib.*\.dll.

This really should have been the default for years but it didn't work properly with ld.bfd: https://github.com/msys2/MINGW-packages/issues/6674
Since it already work good with COFF backend when targeting MSVC I'm strongly in favour of enabling it for MinGW as well.

I don't have write permission, could somebody commit it for me?
Thanks in advance.

@mstorsjo @hans I apologize but we will have to backport https://reviews.llvm.org/D86552 (should affect only MinGW so it's rather low risk) or revert this cherry-pick on LLVM 11 branch.

Indeed, hardcoding z3 was stupid oversight from my side.

Could you commit it for me please? I don't have write permission to the repo.

Could you commit it for me please? I don't have write permission to the repo.

Updated the diff.

Updated diff to remove _MSC_VER change.

Thank you, now it works as expected:

$ cat tools/llvm-config/BuildVariables.inc | grep LLVM_SYSTEM_LIBS
#define LLVM_SYSTEM_LIBS "-lpsapi -lshell32 -lole32 -luuid -ladvapi32 -lz"

Only if weak works properly with GCC as well.