Page MenuHomePhabricator

[libcxx] Never use <cassert> within libc++
ClosedPublic

Authored by EricWF on Jan 23 2017, 5:39 PM.

Details

Summary

It is my opinion that libc++ should never use <cassert>, including in the dylib. This patch remove all uses of assert from within libc++ and replaces most of them with _LIBCPP_ASSERT instead.

Additionally this patch turn LIBCXX_ENABLE_ASSERTIONS off by default, because the standard library should not be aborting user programs unless explicitly asked to.

Diff Detail

Event Timeline

EricWF created this revision.Jan 23 2017, 5:39 PM
mclow.lists accepted this revision.Jan 23 2017, 8:33 PM

this LGTM. assert is not something we should have in the dylib.

include/__config
827

Does this belong here?

This revision is now accepted and ready to land.Jan 23 2017, 8:33 PM
EricWF added inline comments.Jan 23 2017, 8:36 PM
include/__config
827

Yeah, so -DLIBCXX_ENABLE_ASSERTIONS=ON now define -D_LIBCPP_DEBUG=0 during the build. This suppresses std::strings extern template declarations and changes the export lists of libc++.dylib. This change ensures that enabling debug mode during the build doesn't do that.

EricWF closed this revision.Jan 23 2017, 9:08 PM

@EricWF: My downstream builds have -DLLVM_ENABLE_ASSERTIONS=ON by default and they are showing some failures after this commit. Log attached:

It should be pretty easy to reproduce these (I'm on Ubuntu 16.04) if you enable assertions. Since the default has changed, the public builders won't be seeing these.

Are these failures expected?

Thanks.

/ Asiri

Sorry, I meant -DLIBCXX_ENABLE_ASSERTIONS=ON.

It seems like weird usages of _LIBCPP_ASSERT in <string_view> are causing this problem.
Specifically the usages in <string_view> attempt to use _LIBCPP_ASSERT in C++11 constexpr functions and this currently does not work.

For now I've reverted this change in r292923.