This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Don't manually override NDEBUG in the dylib build
ClosedPublic

Authored by ldionne on Mar 8 2022, 12:21 PM.

Details

Summary

LIBCXX_ENABLE_ASSERTIONS does not have any relationship to the assert
macro -- it only controls assertions that are internal to the library.
Playing around with NDEBUG only muddies the picture further than it
already is.

Also, remove a failing assertion in the benchmarks. That assertion had
never been exercised because we defined NDEBUG manually, and it was
failing since we introduced the ability to generate a benchmark vector
with the Quicksort adversary ordering (which is obviously not sorted).

This was split off of https://llvm.org/D121123.

Diff Detail

Event Timeline

ldionne created this revision.Mar 8 2022, 12:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 12:21 PM
Herald added a subscriber: mgorny. · View Herald Transcript
ldionne requested review of this revision.Mar 8 2022, 12:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 12:21 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone accepted this revision.Mar 8 2022, 12:45 PM
Quuxplusone added a subscriber: Quuxplusone.

LGTM if CI is green. But do I understand correctly that this is still two orthogonal patches — one related to LIBCXX_ENABLE_ASSERTIONS, and one completely orthogonal to that in the benchmark code? Prefer to land in two separate git commits, just for hygiene.

This revision is now accepted and ready to land.Mar 8 2022, 12:45 PM

LGTM if CI is green. But do I understand correctly that this is still two orthogonal patches — one related to LIBCXX_ENABLE_ASSERTIONS, and one completely orthogonal to that in the benchmark code? Prefer to land in two separate git commits, just for hygiene.

Nope -- those are related. Basically, when we stop defining -DNDEBUG, the assertion is not a noop anymore, and it fails (for the quicksort adversarial vector). Removing the manual setting of NDEBUG requires removing the assertion (which has been faulty for a while now, but we never noticed).

This revision was automatically updated to reflect the committed changes.