LGTM, although you still have to rebase it past the change to camelBack casing of variables :-)

Thanks! Seems to build fine for me now.

In D59995#1585031, @fhahn wrote:

In D59995#1584967, @mstorsjo wrote:

This triggers failed asserts in some cases, reproable with https://martin.st/temp/loadimage-preproc.cpp.xz, with clang++ -target i686-w64-mingw32 -c -O3 loadimage-preproc.cpp. Will post a proper bug report later today, unless someone else beats me to it.

Thanks for reporting the issue, should be fixed in rL366049.

This triggers failed asserts in some cases, reproable with https://martin.st/temp/loadimage-preproc.cpp.xz, with clang++ -target i686-w64-mingw32 -c -O3 loadimage-preproc.cpp. Will post a proper bug report later today, unless someone else beats me to it.

Thanks for the quick action, this is almost perfect! In my env I had to add an explicit stddef.h include (which didn't cause trouble with use of free) in order to get a typedef of size_t.

Sorry for not testing out this commit until it hit the tree and not commenting on this before. This one turned out to break MinGW builds for a few different reasons, that I'll comment on inline.

Any other concerns with the patch, or is it ok as is?

Added the comment Rui requested. I also renamed the obj file from .obj to .o, as it is created with windres, not cvtres.

In D63109#1557242, @thakis wrote:

I think all four combinations of {llvm-cvtres,cvtres.exe} x {llvm-link,link.exe} should have the same behavior. What does link.exe say to two windres outputs? What does GNU ld do with two cvtres.exe inputs?

LGTM

I'd suggest skipping the lld condition, the version script adds no value here.

Actually, it turns out that GNU windres does things differently - GNU ld can link two resource object files from GNU windres, and resources from both end up reachable. When faced with two resource files from llvm-cvtres, only resources from the first one ends up accessible. In the converse case, if lld is given a resource object file from GNU windres (even a single one), those resources aren't reachable at all.

In D63109#1555283, @thakis wrote:

Sorry for not replying, I'm on vacation.

Actually, I'm leaning towards making this only a warning in MinGW cases. If multiple resource objects are passed and the .rsrc$01 sections are concatenated, is the effect that only the first one actually is read and used, or what does happen?

link.exe's /wholearchive semantics on the other hand mean that it wouldn't load the resource object files from the .lib file at all. Since this scenario is probably still an unlikely corner case, the difference in behavior here seems fine -- and lld-link might have to change to use link.exe's /wholearchive semantics in the future anyways.

In D62625#1553942, @reames wrote:

In D62625#1553095, @mstorsjo wrote:

Compile time regression. (I build the project it's part of with a 120 second timeout per translation unit, enforced with ulimit -t 120, that's why I noticed.)

If you could provide an IR reproducer, and ideally a hint where to look, I can see what I find. Please file a bug with reproduction instructions and an IR attachment.

In D62625#1552791, @reames wrote:

In D62625#1552681, @mstorsjo wrote:

I just ran into a performance regression due to this commit. With https://martin.st/temp/glew-preproc-i686.c, compiling with clang -target i686-w64-mingw32 -c -O3 glew-preproc-i686.c went from 73 seconds before this commit, to 133 seconds afterwards.

Clarification here. Do you mean this was a compile time regression? Or a runtime regression? (i.e. is your clang here build with a different compiler? Or is this a self host?)

I just ran into a performance regression due to this commit. With https://martin.st/temp/glew-preproc-i686.c, compiling with clang -target i686-w64-mingw32 -c -O3 glew-preproc-i686.c went from 73 seconds before this commit, to 133 seconds afterwards.

In D63109#1545204, @mstorsjo wrote:

FWIW, I ran into one error with this on VLC. In one case, a static library ends up (erroneously) containing a resource object file. This static library is included with the --whole-archive option (indirectly, via lots of libtool magic) when linking a DLL, which also contains a resource object files of its own.

In that case, it's possible to fix the issue by removing the intermediate static library which seems to be pointless at the moment. But extending lld to ignore resource object files when linking with --whole-archive would probably be good.

FWIW, I ran into one error with this on VLC. In one case, a static library ends up (erroneously) containing a resource object file. This static library is included with the --whole-archive option (indirectly, via lots of libtool magic) when linking a DLL, which also contains a resource object files of its own.

This caused a regression, where compilation now crashes. See https://bugs.llvm.org/show_bug.cgi?id=42279 for details.

The only real way to run into this is if users manually convert .res files to .obj files by running cvtres and then handing the resulting .obj files to lld-link instead, which in practice likely never happens.

Avoid expanding unless no -m option was found.

Expand only in isPETarget, expand in the mingw driver as well.

Localized the extra expansion to the ELF case, use the host platform native quoting style.

In D63024#1535958, @ruiu wrote:

I'm not particularly worried about the performance impact of doubly-expanding response files. If it is proved to be too slow, we need to do something, but until then I wouldn't optimize it.

But at least we can expand response files only for GNU flavor. For COFF and WASM linkers, we don't have to expand them at this stage.

In D63024#1535947, @ruiu wrote:

In D63024#1535918, @mstorsjo wrote:

In D63024#1535891, @ruiu wrote:

Do you have to support --rsp-quoting for the use case that you want to support?

Not for my use case, no. But Reid wanted to move all expansion of response files out from the individual linkers here, to avoid reading (and allocating memory for) the (potentially large) response file twice. And in that case, we must handle --rsp-quoting perfectly here.

Each flavor's link() function is a public interface, and each function needs to do what the command would do when the same argument strings are given. That means we need to expand response files when the linker is invoked by lld::{coff,elf,wasm,..}::link().

In D63024#1535891, @ruiu wrote:

Do you have to support --rsp-quoting for the use case that you want to support?

In D63024#1534763, @rnk wrote:

For links that use response files (i.e. most of them) this effectively leaks a second copy of the entire response file. Every argument needs to be put in the string saver so it can be null terminated. Response files tend to be large enough that I'd prefer it if we didn't do that.

One way we could address this is to try to calculate the flavor twice:

• First, use the unexpanded arguments to guess the flavor
• Use the guessed flavor to expand response files: Windows tokenization for lld-link, GNU for everyone else
• Re-parse the flavor using the expanded arguments, and call the appropriate entry point with those arguments
• Remove existing rsp expansion code

Adjusted to make it work more like how this is implemented in the ELF linker; still failing (and properly so) if there's real undefined references to a symbol that was requested with the new option (-u in GNU ld).

In D62976#1533841, @ruiu wrote:

I guess that my previous comment addresses the issue?

I don't know this area so I can't comment on it from that perspective, but it does indeed speed up my case, to even faster than it was before the regression. On my system, it originally took 75 s to compile, 220 s after the regression, and now 62 s with this patch. So looking good in that aspect at least!

Looks good and thought through. I had a few nits though.

Mimic the real -u option behavior properly.

This caused a serious compile time regression. https://martin.st/temp/glew-preproc.c used to take 75 s to compile, now it takes 220 s. Repro with clang -target x86_64-w64-mingw32 -c -O3 glew-preproc.c.

Can you add a testcase for this? Also it's appreciated if the uploaded diffs are made with a larger context (diff -U999 or similar) to allow expanding the context here in phabricator.

This change had another breaking effect as well, for the MinGW target. Code that implements a COM interface easily ends up overriding a __declspec(nothrow) function with a method that in most cases (at least in cases that follow the microsoft sample code) lacks the same attribute. For code with -fms-extensions, this is a warning (as all exception spec mismatches are non-fatal there), but MinGW code doesn't necessarily use this option. See https://bugs.llvm.org/show_bug.cgi?id=42100 for full issue report and discussion.

This change broke compiling Qt on MinGW, see https://bugs.llvm.org/show_bug.cgi?id=42089. Trivially reproducible by trying to compile a snippet that looks like this:

Please add testcases for these changes. You could e.g. change some of the existing tests in test/tools/llvm-rc so that both forms are exercised, or add a new test there that uses that form.

Testcase and slightly longer description is available at https://bugs.llvm.org/show_bug.cgi?id=42065.