Actually, this might not be needed? https://cmake.org/cmake/help/v3.4/manual/cmake-policies.7.html says

Hmm, I'd done this as part of https://reviews.llvm.org/D39431, but then https://reviews.llvm.org/D50407 erroneously removed it.

In D60470#1467862, @MaskRay wrote:

In D60470#1467587, @t.p.northover wrote:

Have you run make check on this patch?

If you meant llvm-objdump -exports-trie. It was my problem and I apologize for the llvm-objdump test failure I caused. Or https://reviews.llvm.org/D60376 where I didn't notice BSD sed's escape character support for y command is different from GNU sed on Linux. ("The meaning of a <backslash> followed by any character that is not 'n', a <backslash>, or the delimiter character is undefined."). Thank you for telling that there is ninja check beside check-all check-llvm ..., but those discussions are unrelated to this revision.

Review comments

Review comments

LGTM, thanks!

In D60577#1463552, @srhines wrote:

In D60577#1463469, @smeenai wrote:

I have zero context on this change specifically, but "we should prefer functions in a private namespace" seems to be the exact opposite of the advice in https://llvm.org/docs/CodingStandards.html#static

While the commit comment references possibly moving things to a private namespace (which I agree is in contradiction to the referenced page), the actual code moves these to being private member functions. For functions that are taking *this, or propagating a *this in all usage, it seems fairly appropriate to me. Updating the commit message to not disagree with the official guidance would be helpful.

I have zero context on this change specifically, but "we should prefer functions in a private namespace" seems to be the exact opposite of the advice in https://llvm.org/docs/CodingStandards.html#static

LGTM, thanks.

Thank you!

LGTM. Thanks for doing this!

In D60309#1456569, @ldionne wrote:

While we're at it, do you know why the LIBCXX_ENABLE_ABI_LINKER_SCRIPT isn't allowed on Apple platforms? It doesn't seem to me like this is something you'd want to necessarily prevent on Apple platforms.

(adding the correct Phabricator account for Rui)

I really like the direction this is going.

LGTM with the comment added.

LGTM

(to be clear I also think it's fine to unbreak this first and do the find_package change later, but the latter should hopefully end up being much nicer)

LGTM, though I agree with @ldionne, and I'll wait for him (or another libc++ maintainer) to accept.

Any updates here?

Rebase

Address review comments

Standardize COMMENT

Sorry, I went down a rabbit hole reviewing this, and ended up writing an alternate solution which just generates a native llvm-nm using the same mechanism as tblgen. Gimme a little bit to put that up.

Looks good. I'll commit this.

Also, it might be worth adding a third level of struct to the test, to show that it handles arbitrary nesting correctly (which it does).

Do you need someone to commit this for you?

@ab, @mtrent – this is a small patch, and I can keep it locally if need be, especially if all the arm64e work will be upstreamed soon. With that said, I'd really appreciate any input on whether this is okay or not from the perspective of conflicting with Apple's planned arm64e upstreaming, and what the timeline on that upstreaming might look like. Also CC'ing @ributzka, since TAPI was my motivation for adding this.

In D59727#1442687, @mstorsjo wrote:

FWIW, this will break mingw builds that use the old msvcrt.dll. Since a couple years, mingw-w64 also supports the UCRT, but this is not the default configuration upstream yet (while it is in my own builds).

Ping @ab. Any objections to me landing this?

I'd recommend keeping the projects out-of-tree and using LLVM_ENABLE_PROJECTS instead.

Thank you! This does even better than gold now :)

In D59649#1438809, @MaskRay wrote:

In D59649#1438330, @smeenai wrote:

Thank you!

If I'm understanding correctly, for the case where the relocation is not in the same object file as the discarded section, we'll still get the "undefined symbol" error message. I don't know if it would be possible to enhance that case to print the "relocation against discarded section" message as well. (gold does print that message in this case; it just doesn't print the additional information about the section group signature and prevailing definition.)

Symbol has the File field but not the section index (st_shndx). If the relocation is not in the same object as the discarded section, we can't get the defined section easily.

Thank you!

In any case, I apologize for the confusion, @thomasanderson – this isn't on you :)

@mclow.lists – this is a small localized change specific to libc++'s Windows support. The only libc++ specific thing here is the use of the _LIBCPP_MSVCRT conditionals, but they're consistent with the rest of this file (and with the general intent to support non-msvcrt runtimes on Windows). I've reviewed and contributed a bunch of libc++ Windows support, so I considered myself to be a qualified reviewer for this change, although for this particular change I think anyone sufficiently familiar with msvcrt would be able to review it.

Unrelated, but out of curiosity: I believe libc++ is set up to require building against the Windows 10 SDK and the Universal CRT right now. For older Windows versions, does Chrome require the Universal CRT to be installed?

LGTM, though you'll wanna update the commit message.

Ping @ab. Any objections to me landing this?