In D52344#1242530, @rjmccall wrote:

In D52344#1242451, @kristina wrote:

Would you be okay with me renaming cfstring to .cfstring for ELF so it's in line with ELF section naming conventions (I'm not entirely sure if that could cause issues with ObjC stuff)? And yes I think it's best to avoid that code-path altogether if it turns out to be a constant as that's likely from the declaration of the type, I'll write a FileCheck test in a moment and check that I can apply the same logic to ELF aside from the DLL stuff as CoreFoundation needs to export the symbol somehow while preserving the implicit extern attribute for every other TU that comes from the builtin that CFSTR expands to. Is what I'm suggesting making sense? If not, let me know, I may be misunderstanding what's happening here.

Following the ELF conventions seems like the right thing to do, assuming it doesn't cause compatibility problems. David Chisnall might know best here.

All right, those are all fair arguments. LGTM.

The existing capitalization (as in before your change) is the correct one for these headers in the Windows SDK, and it's also possible to cross-compile against the Windows SDK on a case-sensitive filesystem. However, doing that cross-compilation requires using a VFS overlay or case-correcting symlinks or ciopfs or whatever anyway, because the casing in the rest of the Windows SDK is hopelessly broken, so I'm okay with this change. I'll let @rnk confirm that it's okay to change the capitalization to the non-Windows SDK form though, and I'm also CCing @zturner since he also did cross-compilation work against the Windows SDK on Linux.

It's lowercase in the Windows SDK as well (or at least in versions 14393, 16299, and 17134), so this LGTM.

You'll wanna upload the patch with full context (http://llvm.org/docs/Phabricator.html).

You can still use VirtualProtectFromApp with UWP; you just can't do W+X.

D51374 has the mailing list. Sorry for the noise.

Argh, why didn't Herald add the -commits list?

@beanz did this in https://reviews.llvm.org/rL340436

Sorry for the belated response; I was on vacation.

In D44501#1200324, @mstorsjo wrote:

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

This is pretty confusing, but there's both LLVM_ENABLE_PROJECTS, which assumes the side-by-side layout, and LLVM_EXTERNAL_PROJECTS, which assumes the corresponding SOURCE_DIR variables will be set. It sounds like LLVM_EXTERNAL_PROJECTS is what you want for your use case.

Also, this ended up going to llvm-commits instead of cfe-commits, which isn't ideal.

Patch is missing context.

This went out to llvm-commits. You may wanna re-upload with cfe-commits added instead.

Sorry this has been sitting for so long. I have some questions, and I'll also try to play around with this locally because I don't fully understand some parts.

Seems like an obvious enough fix, so I'm jumping in to LGTM.

LGTM

I'm not familiar enough with this to review it.

Actually, I lied; bfd and gold appear to print their full path. gcc does print the basename though, and that seems like a good compromise.

In D49189#1159171, @ruiu wrote:

Maybe printing out the basename of argv[0] could get the right balance. What do you think?

In D49189#1159019, @ruiu wrote:

I think we should at least include "ld.lld: " part in the error message, because without that, it is sometimes hard to identify what command is emitting an error in a long build log.

Microsoft's implementation also checks the stack allocation against the thread's stack limit. Do you care about doing that?

LGTM, particularly given r314138.

LGTM

In D48694#1148718, @EricWF wrote:

For whether it makes sense to search for includes in the system path, that boils down to @ldionne's question of whether building libc++abi against a system-installed libc++ is supported. I actually don't know the answer to that myself ... @dexonsmith and @EricWF, what are your thoughts on that? The current search won't find the system-installed libc++ headers on Darwin anyway though, where they're placed in the compiler's include directory rather than a system include directory.

Building libc++abi against an installed libc++ doesn't make sense IMO, so I don't care if we try to support it. Libc++abi is a lower lever component with libc++ being built on top of it.
If you want to update libc++abi, you should be building a new libc++ as well. (Note: Using system libc++abi headers to build libc++ would make sense though).

As an aside, libc++abi should really live in the libc++ repository. That way it would always have the correct headers available. But every time I pitch that idea I get a ton of push back.

In D48694#1146232, @EricWF wrote:

LGTM, but I'm a bit confused. You seem to argue that no system places C++ headers on the default search paths, but also that it would be a problem if such a system did.
Why wouldn't the conclusion be true? That is, although C++ includes aren't normally found along the default paths, when they are found, we should still consider them?

That being said, the current behavior of searching certian default include paths first does seem incorrect. So I'm OK with disabling it.

I also agree that we shouldn't necessarily be looking for vector header, how about looking for __config instead, since it's libc++ specific?

LGTM

This doesn't map to any command line change; it's purely a CMake thing. It just changes where libc++abi's build system looks for libc++ headers. Before this patch, it would look for a file named "vector" in all the standard system include directories (/usr/local/include, /usr/include, anything else in your PATH, etc.) and then look for it in the paths specified by the PATHS argument. After this patch it will only look for that file in the paths specified by the PATHS argument. The actual value of LIBCXXABI_LIBCXX_INCLUDES gets added to the header search path when compiling the libc++abi sources, but this diff isn't changing that part (it just potentially changes the value that gets computed for LIBCXXABI_LIBCXX_INCLUDES, but as I detailed in the description I don't think that should actually change on any system I'm aware of, and if it does cause a change then there's a good chance the old value was incorrect).

D48694

Huh, this went to llvm-commits instead of cfe-commits automatically. I guess the auto mailing list subscribing doesn't work great with the monorepo?

Adding some people who are interested in Windows cross-compilation.

This is definitely trivial enough to just go through post-commit review :) (As in, just commit it directly without going through Phabricator.)

Okay, upon playing with this further, the following seems to do what I want (for a custom sysroot), and seems to be a pretty common pattern as well:

Ah, the documentation is confusing, but CMAKE_FIND_ROOT_PATH and CMAKE_FIND_ROOT_PATH_MODE_PACKAGE only have an effect when using the config mode of find_package, whereas this invocation is using the module mode. That's a bummer, and definitely makes custom sysroots and cross-compilation harder than it should be. Sorry for the noise.

Actually, I would imagine that if you're cross-compiling or using a custom sysroot, you should probably also specify CMAKE_FIND_ROOT_PATH and set CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY, to limit all these searches to the desired directories?

I think it might be more appropriate to pass NO_SYSTEM_ENVIRONMENT_PATH to find_package in case CMAKE_SYSROOT or CMAKE_CROSSCOMPILING are set? That way, we won't search the host system for libxml2 in those cases, but we'll still be able to find it in the custom sysroot or cross-compilation SDK if it's present.

In D48402#1141048, @pirama wrote:

In D48402#1140046, @rnk wrote:

In D48402#1140019, @pirama wrote:

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD

Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

I replaced getMangler().getNameWithPrefix(SecName, Symbol, DL); in appendComdatSymbolForMinGW with SecName.append(Symbol.begin(), Symbol.end());. This removes the extra underscore but still doesn't associate a comdat with the section. Is there anything else that I can try?

LGTM with the same comments as D48356 (add quotes, and is the translation for LLVM_BINARY_DIR necessary?)

LGTM with the same comments as D48356 (add quotes, and is the translation for LLVM_BINARY_DIR necessary?)

Looks good with the quotes added.

Ping.

Ping.

LGTM

This new version is much better; there doesn't actually appear to be any performance difference for my test case at all now (barring noise). Thank you!

This diff results in a 30% link time regression for us (5.3 seconds to 6.8 seconds) when linking ~5,000 input files together. This is when cross-compiling on Linux; the results would probably be even worse on Windows, where I believe stat/the filesystem in general is slower.

In D47233#1129810, @DHowett-MSFT wrote:

In D47233#1129207, @rjmccall wrote:

In D47233#1129110, @smeenai wrote:

WinObjC does this wrapping, for example.

I see. The idea of creating the type descriptors and mangled names ad-hoc for the catchable-types array is clever, and it's nice that the ABI is defined in a way that makes that work. (Expensive, but hey, it's the exceptions path.)

We ran into some critical issues with this approach on x64. The pointers in the exception record are supposed to be image-relative instead of absolute, so I tried to make them absolute to libobjc2's load address, but I never quite solved it.

A slightly better-documented and cleaner version of the code you linked is here.

Wrapping an Objective-C exception inside a C++ exception means dynamically constructing a C++ exception object and traversing the class hierarchy of the thrown Obj-C object to populate the catchable types array of the C++ exception. Microsoft's C++ runtime will perform the appropriate catch type comparisons, and this patch makes the compiler emit the right typeinfos into the exception handling tables for all of that to work. https://github.com/Microsoft/libobjc2/blob/f2e4c5ac4b3ac17f413a38bbc7ee1242f9efd0f7/msvc/eh_winobjc.cc#L116 is how WinObjC does this wrapping, for example.

Makes sense to me, but I want @ruiu to look over it.

From what I understand, the way LTO works on LLD COFF and ELF is that the appropriate LLVM libraries are just (statically) linked into LLD itself, so there's no concept of an LTO library. I assume that LTO support in LLD Mach-O will work the same way when it's implemented, so the option should never be needed there either. I think it makes more sense to change the clang driver to not pass the option to LLD, but I'm adding @pcc to confirm that my understanding of LTO is correct.

@rnk remember how I was asking you about inlining into catchpads on IRC a few days back? It was in relation to this change.