Address feedback. Also fix the case that was previously not working. This now covers all the various cases that have been discussed.

Ah, thanks for the explanation @efriedma.

This is a step in the right direction. Although the NSDMI cases and default parameter value cases are not yet handled, they break due to tracking of the global mangling number tracking, not due to the scheme.

@efriedma I think that Im still not understanding the case that you are trying to point out. How do we end up in a state where the block invocation function is referenced external to the TU? The block would be referenced to by name of the block, no? AFAICT, this is for local storage in a block, which is scoped to the block invocation function, which is TU local, and will be referenced by the block_literal (which contains the block invocation function and is dispatched via the BlocksRuntime).

@efriedma which bit of the Itanium mangling should I be looking at? A BlockDecl does not have visibility associated with them, so Im not sure what I should be checking to see if the block is visible or not. What is the best way forward for finishing this up?

Add additional test cases, improve coverage and mangling.

@efriedma hmm...using getBlockManglingNumber causes the name to be duplicated. Ill look into that. However, wouldn't all the block invocation functions be defined and COMDAT'ed?

SVN r304670

@ismail the current version of the patch should have those requested changes :-).

Im not sure that this is a reasonable patch. I think that specializing a AddressSpace for _WIN32 is more likely what we want. Furthermore, this actually should be more restrictive, since the unwind library cannot be used for Windows -- the unwinding model on Windows is completely different. This only makes sense for MinGW and cygwin, and so we should ensure that the environment matches that.

Thanks for looking into this, its been on my list for a while now.

Please clang-format before committing this.

Looks generally pretty good. This is going to be a pretty cool addition!

Modulo the IT block macros, LGTM.

Can you please ensure that you cross-port this into llvm/lib/Demangle/ItaniumDemangle.cpp?

Sure, a _LIBCPP_MSVCRT_LIKE WFM. I just want to make sure that we don''t conflate the underlying libc implementation with the Win32 API set.

I think that we should sink the min/max checks into __undef_macros. I don't like the idea of littering that check everywhere.

@majnemer thats the idea - passing either triple to clang would give the right behaviour (clang will normalize the triple before passing it to cc1).
@ismail Im waiting on you to accept the change

@mstorsjo Yeah, clang supports proper TLS on Windows on ARM. This is only needed if you are explicitly going out of your way to use the emulated TLS (via -femulated-tls). Otherwise, this code path shouldn't be hit.

All of this functionality is in the current tool AFAIK. Furthermore, this has been out of date for so long that it's not particularly useful anymore. Commandeering to close the differential.

Doing this on just the MinGW build seems reasonable. Most of the library shouldn't be needed for Windows.

@majnemer thats the idea :-). Passing either triple to clang would give the right behaviour (clang will normalize the triple before passing it to cc1).

Yes, the idea is that the textual IR uses the correct triple, unless it is overidden at the command line.

Why not always replace the extension? Windows doesnt require the .exe suffix IIRC.

Is this to actually get the correct GCC search dir? Your test doesnt really test anything AFAICT, as it is just invoking clang with a target that it would accept anyways.

I think that you should mutate the environment in the canonicalisation phase of the triple. That will allow you to use armv7-suse-linux-gnueabi and armv7-suse-linux-gnueabihf in the frontend, but have the backend always get armv7-suse-linux-gnueabihf.

This would ideally wait for the change that @jbcoe has in the works to enable python 3, but the change itself is fine.

Thanks for doing this cleanup!

I think it would've been nice to split this up into the changes for map/filter rather than group it together. But sure, this looks reasonable.

Please add a test case. The change itself looks reasonable.

This is opt-out by default, so it shouldn't have any impact on users by default. When building for tooling, this should allow smaller builds for those.

Minor comments on the test, but LGTM otherwise.