r308492

Add comment

Ping. I updated the description based on the email discussion with @majnemer.

Thanks for the explanations @rafael. Everything makes sense now.

(from @rafael's email) Correct, bfd complains. That is why you have to make foo weak, which is annoying.

I'm probably just misunderstanding the discussion here, but bfd complains about multiple definitions if you do .symver x, x@@VER: https://ghostbin.com/paste/c9b4t. Granted, I'm on an old version of binutils, so maybe that's changed?

arc snafu

Correct formatting

Add comment for isDWARF

You probably wanna update the summary. We also generally recommend uploading patches with more context (pass -U9999 to your git commands), though it doesn't make much difference over here.

@thomasanderson you could re-upload this diff with just the include/vector change, and then @EricWF can decide if that's the right thing to do.

@peter.smith Yeah changing compareByFilePosition should work for now, though stable_sort might be more future-proof. I'll leave that for @rafael and @ruiu to decide.

I discovered this because the .ARM.exidx end sentinel was getting incorrectly reordered away from the end of the section, leading to unwinding failing in certain binaries. CC @peter.smith, in case you've run into any similar issues.

In D35326#808884, @thomasanderson wrote:

In D35326#808870, @smeenai wrote:

r307966 fixes string operator+, and D35388 fixes __vector_base_common.

Awesome, thanks for the quick fixes!

In D35326#808855, @thomasanderson wrote:

Also, string has a class almost identical to __vector_base_common:
https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/string;307970$560

But currently string has _LIBCPP_TEMPLATE_VIS and vector does not. Maybe at least one of them is wrong?

r307966 fixes string operator+, and D35388 fixes __vector_base_common.

Hmm, that's pretty ancient (and I would be surprised if that changed in newer versions). Lemme play with this some more.

Hmm, what version of gcc are you using? https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/docs/DesignDocs/VisibilityMacros.rst;307951$86-90 suggests this may have changed recently.

Perfect. I'll throw up some patches.

Does P8004 fix operator+?

This doesn't look right to me. _LIBCPP_TEMPLATE_VIS expands to __attribute__((__type_visibility__("default"))) on compilers where __type_visibility__ is supported; i.e., its intent is to only export the typeinfo and vtable. gcc doesn't support this attribute, so we'll use __visibility__ instead and this change will appear to fix your issue, but it's not conceptually correct.

COFF supports weak externals: https://stackoverflow.com/a/11529277/382079. Would it suffice here?

(adding some Windows and cmake people)

Sorry about the breakage. I'll redo this limiting it to windows-itanium (and add a comment explaining why we want it).

Sorry, I've been meaning to respond for a while.

LGTM, but @ruiu or @rafael has to approve, of course. I'm happy to see this though :)

Thank you so much! This will be really helpful for me.

This looks sensible to me. I don't know if there are any scenarios in which you'd be using the Microsoft CRT without having _MSC_VER defined, however. Added some people who may have a better idea of that (@compnerd, @majnemer and @rnk)

Will just drop the .exe entirely, since that works

Thanks!

LGTM. Thanks for all the revisions!

@sbc100 Normally you'd be able to accept the revision as an explicit LGTM. In this case, the revision had already been committed, so you don't have that option anymore.

In D33082#754451, @compnerd wrote:

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.

@compnerd see my previous comment:

@EricWF and I discussed this on IRC a bit. I'm not a fan of overloading _LIBCPP_WIN32API for this purpose, since to me that macro is meant for guarding Windows API functions, not for CRT functions. Eric suggested adding a new macro _LIBCPP_MSVCRT_LIKE, which I'd be fine with.

Confirmed that there are no ABI changed on both Linux and macOS.

Also ABI list checks for libc++abi sound awesome.

Fair enough. I'll probably get to that tomorrow.

Ping.

In D32652#741088, @ruiu wrote:

I feel like we should fix COFFObjectFile::getSectionContents to not fail on empty sections instead of avoid calling it on empty sections, as I naturally expect that getSectionContents returns an empty StringRef for an empty section.

Empty sections causing a parse failure strikes me as slightly odd, but I'm guessing there's a good rationale for it. @compnerd?

Ping.

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

Thanks for the review!

Thanks for the review and for adjusting the reviewers. I'm requesting a review again so that it shows up in the right queue for @rafael and @ruiu.

Reduce nesting (address comments)

This looks good to me, but I'm not familiar enough with this part of clang to be comfortable accepting.

Forgot to fix a test

Add documentation