@lattner Do you want to follow up on this one? StringRef already has a bunch of case insensitive operations, and this patch just fills a gap in the set of those operations.

Use @rnk's suggsestion of isa<BitcodFile>(), simplify the initialization of leaderChunk and leaderSelection.

Updated to rerun CI after fixing a test in the base patch

Updated to rerun CI after fixing a test in the base patch

Updated missed bits in one testcase.

Unconditionally printing the command to stderr.

In D104526#2827401, @rnk wrote:

To actually test the effect of this, we'd have to actually do linking
from the test/MinGW subdirectory (which we don't do so far) and
have that link trigger an error.

Do you think it's necessary to make a standalone failing link test
to test the effect of this, or is it good as is?

I think mingw lld can certainly have integration tests. I think it's better to focus on testing at the smallest testable boundary, which is the argument translation layer, but tactical integration testing is also valuable.

I don't think it's strictly necessary in this case, but if you'd like to add one, go for it.

Looks ok to me. A PE-COFF image is limited to 32 bits in range anyway, so as long as the target symbol doesn't turn out to be from outside of the image (with mingw automatic dllimport), a 32 bit difference should be enough in all cases.

Squashed in two changes to two tools that can get rid of their own private helper function if this is hoisted to StringRef.

Furthermore, there is apparently basically no downside to building the libraries with position independent code, since modern toolchains are sufficiently clever.

In D104218#2820315, @lattner wrote:

Oh, I completely missed that. I would recommend using the word case_insensitive here. I don't think stringref has any other case insensitive support, and I don't think this is the obvious place to start. I would start with case insensitive equality comparison. Your client can use that as a primitive that it builds higher level functionality on top of.

In D104218#2819837, @lattner wrote:

I was thinking that the closure would take a character, not a stringref. So you could do something like str.consume_back(islower).

LGTM

In D104218#2817946, @MaskRay wrote:

Can you show a patch which will benefit from this? Otherwise this looks good.

In D101479#2813797, @phosek wrote:

In D101479#2813724, @mstorsjo wrote:

Couldn’t this commit have been kept in, and just reverting the one for using it in the fuchsia cmake cache? (I’m not using this particular commit myself, just observing.)

The problem is that there's no way to configure the default libc++ on a per-target basis. We want to use libc++ as a default for all targets that support it, so for example even if you're compiling on Windows but targeting Fuchsia or Linux. Unfortunately, with this change that triggers D103947 when compiling compiler-rt and I don't know how to work around it.

I'd point out that we have the same problem for other defaults, for example if you set lld as the default linker on Darwin, it causes issues because Clang tries to use lld.ld64 which is not yet usable. We still want to use lld for all other targets, but there's no way to specify that in CMake today which is something we may want to address.

Couldn’t this commit have been kept in, and just reverting the one for using it in the fuchsia cmake cache? (I’m not using this particular commit myself, just observing.)

Updated with CI testing hooked up. Unfortunately, things don't work quite cleanly:

The problem I'm seeing can be reproduced with this source file: https://martin.st/temp/cdf-preproc.c

FYI I've bisected down a misoptimization to this particular commit

In D103947#2808999, @rnk wrote:

I think we can say that _HAS_EXCEPTIONS=0 is an important configuration.

In D103947#2808941, @ldionne wrote:

It's the bare minimum for a configuration to be considered as supported. We already have a Windows bot, but I'm actually not sure whether it's using MinGW or straight Windows. @mstorsjo will know. Maybe it's trivial to add another Windows configuration to test this?

I'm a bit curious about the root causes of this patch - is it for builds with MS STL or libc++, and what were the actual issues it was trying to fix? I'm getting mixed signals from the comments in the discussion thread above.

Thanks for looping me in; I don't have any objection to this, as far as I can see, the reasoning seems sensible, and the most brittle testcase I have (from what I remember offhand) seems to run fine with this change.

In D103304#2799816, @kda wrote:

In D103304#2798480, @mstorsjo wrote:

FWIW, this patch also broke building for windows. Weak symbols overall is a tricky area on windows, and I don't think this particular usage (the asan lib having a weak undefined symbol, which the executable may or may not proivide) works with windows DLLs at all.

@mstorsjo Is there a short-term work around?
FYI: I do plan to follow with a late-initialization solution.

FWIW, this patch also broke building for windows. Weak symbols overall is a tricky area on windows, and I don't think this particular usage (the asan lib having a weak undefined symbol, which the executable may or may not proivide) works with windows DLLs at all.

Updated the CodeGenCXX/ext-int.cpp testcase.

Updated to not require the types to be either isAggregateTypeForABI(Ty) or Ty->isMemberPointerType(), just check the size of the type, added a testcase for __int128 (which I tested against GCC).

In D103444#2790915, @Quuxplusone wrote:

Looks reasonable to me. FWIW, a more invasive rewrite would put the letter-computing code first:

In D103370#2791682, @andrewrk wrote:

Thank you for having a look, Adrian! I would appreciate if you or someone else could commit for me since I do not have write access.

+1, looks great to me now, too.

Went with D103310 after all, thus this one isn't needed.

Rerun CI after fixing the failing testcases that this unlocks.

New run