If you use windows-gnu triples Rust will link libgcc_s or libgcc_eh directly unless you modify it's source not to do it: https://github.com/rust-lang/rust/blob/e5fedceabf4e0564231db592b6d1f35e1ca27908/compiler/rustc_target/src/spec/windows_gnu_base.rs#L64

Rust doesn't use LLVM's libunwind for mingw-w64, it relies on libgcc.

So no regression, but for this usecase we placed that particular user of the tool in a tricky spot

I don't have any experience with --use-temp-file so I'll trust your judgement.
Let's get this regression fixed but I'm also curious about the plan for future `--preprocessor changes.

LGTM

LGTM

If we make the flag imply linker options, then it becomes much clearer to spot the error, if you enabled this but the code base still would need autoimports somewhere. (This has happened - see https://code.videolan.org/videolan/dav1d/-/merge_requests/1303#note_301859, https://code.videolan.org/videolan/dav1d/-/merge_requests/1349 and https://code.videolan.org/videolan/dav1d/-/merge_requests/1361.) If we make the flag only affect code generation, it becomes a more clear match for projects using -mcmodel=small with GCC today.

I'm in favor of the warning. I don't know LLVM process for adding new errors for code that was previously accepted (even if wrongly) but IMO new errors should be introduced in a way that doesn't surprisingly break downstreams.
So I'd be okay if this becomes a warning in next release (N) and an error/hard error (don't think LLD threats them differently though) in release N+1 or N+2.

LGTM

LGTM

LGTM

LGTM

In D144084#4140722, @mstorsjo wrote:

I'm not sure what would be best here - add a more specific -tolerate-duplicate-weak-sybols option or something like that? Or add a more specific flag, always passed by default by the mingw driver frontend, e.g. -detect-msvc:no?

Oh, that's unfortunate.
Maybe llvm-windres should generate a warning in such case?

Should that even be considered as supported?

LGTM

I cannot speak much about LLVM tests but the code looks good for me.

In D141206#4043375, @mstorsjo wrote:

Updated with some testcases. This does test that the include directory is omitted when cross compiling, but those kinds of tests, which set up a simulated toolchain environment with symlinks, don't run on actual Windows - so it's not quite as easy to test the case that we actually do keep the unprefixed include directory on Windows. As this matches the current testing coverage status quo, I hope this is ok...

In D141206#4034451, @mstorsjo wrote:

In D141206#4034159, @mati865 wrote:

I had thought we do that already so this change looks reasonable for me.
Just one thought, do we support multilib toolchains? I think in that case Clang would have to add <base>/include despite different triple.

This patch actually does that already, see the parameter RequireArchMatch which is set to true for the <base>/lib case and false for <base>/include.

I had thought we do that already so this change looks reasonable for me.
Just one thought, do we support multilib toolchains? I think in that case Clang would have to add <base>/include despite different triple.

LGTM

LGTM

Thanks, appears to fix unwinding. There are other issues preventing successful compilation but they are seem unrelated.

It's unfortunate that MSVC doesn't distinguish static from import libs by extension but we have to adjust.

Thank you.

LGTM

LGTM

Unfortunately I lack a lot of context here so cannot tell if there are alternative solutions.
How expensive (in terms of performance) is calling updateLinkLoop that many times?

Thanks, confirmed the built compiler still works (bootstrapped it twice with itself) and with all architectures enabled we are at 35289 exported symbols.

I don't know about LLVM policy for such potentially disruptive change but regardless of what is decided here MSYS2 will almost surely backport this change, otherwise there is no way to provide shared libraries.
On the other hand even if it landed earlier it probably wouldn't have received a lot of testing anyway.

As the patches to LLD would be landed before, that's probably fine as long as a user doesn't update Clang without updating LLD.

Sorry, haven't noticed the approval was reset.

LGTM

I'm a little bit concerned this could break some projects in the wild but for projects that use it properly this sounds like huge win (as your LLVM results show).

Even though the changes are small I not sure if approval from an outsider (me) should suffice here.

LGTM with minor question.

lgtm

Confirmed today it solves the problem.

Thank you! It will surely make building easier.

Rebuilt Rust with this patch applied to host LLVM and it still works after stripping binaries and libraries.

LGTM

Looking at Rust's fix over https://github.com/rust-lang/rust/commit/4f0ad1c92ca08da6e8dc17838070975762f59714 seems like there is API added in Windows 10 to solve it (I don't know how effective though).
I'm not going to work on this myself but leaving the link in case somebody else wants to give it a try.

In D68480#3218341, @mstorsjo wrote:

In D68480#3218277, @ldionne wrote:

I will argue that breaking this ABI on Windows is not a big deal since nobody's vending libc++ on Windows. Or at least if they do, they are completely invisible to us ("us" being the people who develop libc++).

Would you be willing to open a patch showing what changes are necessary for each of those? I think we can totally break the ABI on Windows since nobody's shipping it.

I would like to add some nuance to these statements here.

I do ship shared linked libc++ on Windows (as part of the llvm-mingw toolchain distribution), but without any strong ABI guarantees. Ideally ABI wouldn't break between releases, but if it does (especially in a fringe area) it's probably tolerable.

MSYS2 also ships an environment where libc++ is the standard system C++ library, linked shared. That environment is considered somewhat experimental afaik (CC @mati865), so if it breaks some package (which can be fixed by rebuilding), I think they'd consider it tolerable. If none of their packages actually have ended up using and depending on the aspects that may change (I haven't followed closely exactly how widespread it is), the ABI break should be totally transparent though.

There was similar differential recently: https://reviews.llvm.org/D112093

I never had to dig into subsystem versions and what do the mean but looking at subsystem docs 4.0 stands for kernel mode drivers which looks wrong to me. They also specify 5.01 as 32-bit, 5.02 as 64-bit and 6.0 as being either 32-bit or 64-bit.

Agree, we should promote using working and tested configuration.
Even if pthread build works it's probably affected by threading issues when built with GCC.

LGTM

LGTM

LGTM

From a quick look at the options listing, I presume most other cases of S<> and J<…=> should be changed into Eq<> too.

Building only X86 target did the trick and it looks good:

$ ls bin/ | rg libLLVM.*dll
libLLVM-13git.dll

I had no internet during the weekend (massive storms over here) but so I returned to it yesterday.
I'm unable to build clean main branch at the moment:

`
[2972/3064] Linking CXX shared library bin\libLLVM.dll
FAILED: bin/libLLVM.dll lib/libLLVM.dll.a
cmd.exe /C "cd . && D:\msys64\clang64\bin\c++.exe -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,--gc-sections -shared -o bin\libLLVM.dll -Wl,--out-implib,lib\libLLVM.dll.a -Wl,--major-image-version,0,--minor-image-version,0 @CMakeFiles\LLVM.rsp  && cd ."
lld-link: error: too many exported symbols (max 65535)
`

It's simple change though, should I update diff without testing?

LGTM sorry for the delay.

In D89009#2882846, @mstorsjo wrote:

In D89009#2857310, @mati865 wrote:

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

@mati865 - Do you want to update the patch with an if(UNIX) around the symlink lines? I think we've got agreement that we can move forward with this, and it'd be nice to have it in before the 13.x branch in a couple weeks.

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

In D89009#2857190, @mstorsjo wrote:

The import libraries do seem to use the right name as far as I can see, but there's another aspect I remembered now: This does create unversioned symlinks too (not while building but once you do ninja install). This probably is neat to have, but for cases when things are packaged up in e.g. zip files or something else that doesn't handle symlinks, one might want not to have them. Within llvm-mingw I guess I can have them stripped out before packaging though.

Thank you for returning to this.
I'd love to get confirmation from @hans who suggested it in https://reviews.llvm.org/D87521 that this still seems like the best approach.

LGTM once D104530 is merged.

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.