This broke a few tests for me (generating code that now gives the fail result at runtime).

@rnk - Can you follow up on the discussion above?

In D87379#2274531, @srj wrote:

It seems we made a small mistake in one of the if conditions, will fix it now. Thanks for letting us know

What commit/review has the fixes? This is affecting Halide as well and we'd like to test the fix.

In D87379#2273520, @MeeraN wrote:

In D87379#2273244, @mstorsjo wrote:

Headsup: This broke a number of tests for me. Looking closer into where it changed things erroneously...

It seems we made a small mistake in one of the if conditions, will fix it now. Thanks for letting us know

LGTM

The file that shows the error can be built from https://martin.st/temp/eval.c, built as clang -target armv7-w64-mingw32 -c -O2 eval.c.

The main difference in code, in the function that show the error in one of the testcases that broke, looks like this:

-       movs    r1, #9
-       cmp     r0, #9
-       it      lt
-       movlt   r1, r0
-       bic.w   r0, r1, r1, asr #31
+       usat    r0, #1, r0

Headsup: This broke a number of tests for me. Looking closer into where it changed things erroneously...

Pushed this one now.

In D87521#2270953, @hans wrote:

@hans - Does that sound right to you?

Yes, that matches my experience. But I don't know the backstory here. Maybe it would make sense to have version suffixes also on Windows?

Ping @MaskRay

The commit description talks about making the caching optional, but I don't really see that aspect in the patch - or maybe I'm just not studying it closely enough?

In D86256#2244077, @rprichard wrote:

If the unwinder uses dl_iterate_phdr, then entries that are automatically added to DwarfFDECache would become invalid if the module containing the entry were unloaded. (On Apple systems, DwarfFDECache registers dyldUnloadHook to remove unloaded entries.)

AFAICT, this is also a problem for Windows DWARF EH, which relies on scanning an .eh_frame section. e.g. A stale cachedFDE could point into the middle of an FDE/CIE with an arbitrary outcome. Maybe decodeFDE crashes, or maybe it decodes something that looks like a match. I'm not sure if it can be fixed without disabling the caching. If so, then I'd wonder if the caching is worth keeping. (Bare-metal could use .eh_frame_hdr, and I'm not sure whether Apple systems actually need the full __eh_frame scan.)

The change itself looks good to me, but the comment in lit.site.cfg.in does look confusing to me as well, so it'd be great to have it reworded.

In D87521#2270103, @mati865 wrote:

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

Ok, so if I read code correctly, the suffix gets added here: https://github.com/llvm/llvm-project/blob/9c651c231f3144f53e13cd0a1747589e1b2edccd/llvm/cmake/modules/AddLLVM.cmake#L599-L606

LGTM then! Can apply it a bit later.

In D87521#2269446, @mati865 wrote:

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?

I have very limited knowledge about what is going on but I think this version script controls it: https://github.com/llvm/llvm-project/blob/9c651c231f3144f53e13cd0a1747589e1b2edccd/llvm/tools/llvm-shlib/CMakeLists.txt#L51

In D87539#2269442, @mati865 wrote:

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

In D87521#2269384, @mati865 wrote:

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

In D87539#2269385, @mati865 wrote:

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

This should be ok, I think.

Does the static lib built in this case include any dllexport attributes (that can cause issues if linking it statically into another DLL)?

If the corresponding DLLs built in MSVC mode actually have a version number suffix, should we change the build for MinGW mode to include a number as well? Or is any of the relevant DLLs ever built in MSVC mode at all?

Removed superfluous "sub sp, sp, #0".

This looks good to me! I can push it later.

What's the practical effect of this? I see that a number of libraries already have a lib prefix prepended, like libclang/liblldb - I presume this changes the name of the individual libs (for BUILD_SHARED_LIBS=TRUE configurations)?

In D87448#2266746, @rnk wrote:

Makes sense.

Unwind info for all remaining functions that hasn't gotten it emitted directly is emitted at the end.

Can we make .seh_endproc responsible for emitting the unwind info into .xdata if necessary?

In D87371#2267053, @efriedma wrote:

the length of the synthesized canonical prologue might differ a little from the actual one maybe

Getting the length wrong has caused miscompiles in the past.

In D87418#2266652, @pcc wrote:

How widespread are these build systems that parse help output? (Given that it took until now to discover them, I'd venture "not very".) Maybe it would be better to fix them to explicitly pass -m and/or do something that doesn't rely on parsing help output (e.g. just try the flag and see whether it fails).

Updated to allow cases where the epilog is missing the SetFP opcode.

In D87371#2264423, @efriedma wrote:

Unfortunately, in practice, this isn't ever matched by the code generated by LLVM

Are you planning this as a followup?

Added a comment about the reference for synthesizing the prologue, fixed the homing area stack adjustment check for RegI=0, RegF=0, CR=1, added a testcase for that case and expanded on the comment for that bit in the code.

Added a comment in the operator== about why the label isn't included in the comparison, changed the last condition into an early return like the other ones.

Conditionally producing the save_r19r20_x opcode depending on offset, added a testcase with a too large offset that isn't canonicalized.

In D87363#2264369, @MaskRay wrote:

Also add a missing --strict-whitespace in an existing test that was added explicitly for testing vertical alignment.

Without --match-full-lines, leading and trailing space changes cannot be detected. For // CHECK: a b, you can ensure the spaces between a and b are 2, but you cannot test the indentation.

As a concept, this looks sensible to me, similar to how llvm can be configured with a default triple or how clang can have cmake-configurable built-in defaults for linker/stdlib.

In D86612#2262441, @compnerd wrote:

It is definitely possible to disable SEH via /EHs- though

@compnerd - Any followup on this one?

In D86917#2255845, @filcab wrote:

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

@efriedma Any followup to the discussion above?

In D86889#2248077, @efriedma wrote:

Adding assembler support for the basic opcodes we left out because the code generator doesn't use them seems fine. We could teach the assembler to optimize certain constructs: for example, turn save_regp_x into save_r19r20_x if the register matches and the offset is in range. Might be more confusing than helpful, though.

Added a testcase with a tiny python script that generates def files that export the same symbol a number of times. It generates two 566 KB def files, for testing the case below and above the limit. The successful link case produces a 1 MB DLL and a 8 MB import library.