This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove the legacy debug mode.
ClosedPublic

Authored by var-const on Jun 23 2023, 5:05 PM.

Diff Detail

Event Timeline

var-const created this revision.Jun 23 2023, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 5:05 PM
Herald added a subscriber: arichardson. · View Herald Transcript
var-const updated this revision to Diff 534135.Jun 23 2023, 5:31 PM

Compilation fixes.

ldionne published this revision for review.Jun 26 2023, 1:02 PM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/CMakeLists.txt
57

Not attached: We should add a release note.

59

Not attached: I think we need to modify the CI setup to remove the debug mode job (and the associated CMake cache).

libcxx/include/__algorithm/unwrap_iter.h
41

This raises an interesting question -- how do we make sure not to forget to come back and re-evaluate what we should do here in the future? I think we should leave some kind of TODO(debug-mode) behind or something like that.

libcxx/include/__format/buffer.h
256

Same here -- those seem like things we'd want to do with our ownership checking and bounded iterators as well?

libcxx/include/vector
1736

A part of me would be tempted to leave those assertions in place like this, just for documentation:

_LIBCPP_LEGACY_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this, "vector::erase(iterator) called with an iterator not referring to this vector");

This would expand to nothing, but it would document what kind of checking we used to do with the debug mode and would inform what we might want to check in the future, when we get there. WDYT?

libcxx/utils/libcxx/test/features.py
310

You should make a pass for the tests that use libcpp-has-debug-mode -- I see a few where stuff like // XFAIL: libcpp-has-debug-mode can be removed.

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 535026.Jun 27 2023, 9:39 AM
var-const marked 5 inline comments as done.

Address feedback.

var-const added inline comments.Jun 27 2023, 9:40 AM
libcxx/CMakeLists.txt
59

Thanks!

libcxx/include/__algorithm/unwrap_iter.h
41

I thought to simply use the delta in this patch, but adding a note SGTM.

libcxx/include/vector
1736

I thought about this, but was leaning towards removing those. We still have the effectively-disabled tests that check these assertions, and we can refer to the delta in this patch as well. But I'm definitely open to restoring these.

libcxx/utils/libcxx/test/features.py
310

Thanks!

Mordante accepted this revision as: Mordante.Jun 27 2023, 10:00 AM
Mordante added a subscriber: Mordante.

Based on the information on discourse no objection against removal in LLVM-17.
LGTM modulo some nits, leaving final approval to @ldionne.

libcxx/CMakeLists.txt
65

This flag is used in docs/DesignDocs/DebugMode.rst, please update that document.

libcxx/docs/ReleaseNotes.rst
81
var-const marked 2 inline comments as done.

Feedback.

var-const added inline comments.Jun 27 2023, 12:44 PM
libcxx/CMakeLists.txt
65

Thanks! I went ahead and removed the file.

This LGTM once CI is green and comments are applied. Please ping me one last time and I'll only look at the diff-since-last-change real quick. Thanks for the patch, we've been wanting to do this for a long time!

libcxx/include/__algorithm/comp_ref_type.h
54

Should this be UNCATEGORIZED_ASSERT instead? Assuming you land the patch that does the renaming first, I think it should.

libcxx/include/__tree
379–380

I think this should just be a _LIBCPP_UNCATEGORIZED_ASSERT for now. It would work today.

Once we categorize this, this could become something like _LIBCPP_INTERNAL_ASSERTION or something like that since it's effectively an internal consistency check to ensure that our implementation isn't broken. We have other instances of that, and we had thought about adding another assertion macro to handle those back then.

libcxx/include/vector
1736

Ok, sold.

libcxx/utils/gdb/libcxx/printers.py
994 ↗(On Diff #535101)

I don't think this is correct -- this apparently pertains to libstdcxx types (per the comment above).

var-const updated this revision to Diff 535597.Jun 28 2023, 6:34 PM
var-const marked 4 inline comments as done.

Feedback

libcxx/utils/gdb/libcxx/printers.py
994 ↗(On Diff #535101)

Thanks for spotting this!

var-const added inline comments.Jun 28 2023, 7:10 PM
libcxx/include/__algorithm/comp_ref_type.h
54

I'm getting a weird CI failure on this line from our custom clang-tidy checks:

comp_ref_type.h:53:9: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
53 |         _LIBCPP_ASSERT(!__comp_(__l, __r),

I don't see how calling __comp_ would involve ADL given that it's a reference (also other calls to __comp_ in this file are not flagged).

How do I run the clang-tidy tests locally in a Docker container? I tried copying the clang-tidy invocation from the CI, but it looks like my local build is missing /llvm/build/generic-cxx23/libcxx/test/tools/clang_tidy_checks/libcxx-tidy.plugin.

ldionne added inline comments.Jun 29 2023, 6:42 AM
libcxx/include/__algorithm/comp_ref_type.h
54

Use export ENABLE_CLANG_TIDY=On before you do run-buildbot in the docker image!

This can be seen done in buildkite-pipeline.yml. This will cause -DLIBCXX_ENABLE_CLANG_TIDY=On to be passed to the CMake, and the plugin to be compiled.

var-const updated this revision to Diff 535847.Jun 29 2023, 9:25 AM

Fix the CI.

Fix the CI.

var-const added inline comments.Jun 29 2023, 10:47 AM
libcxx/include/__algorithm/comp_ref_type.h
54

Thanks! Looks like the file was simply missing the <__assert> include so that clang-tidy must have thought that _LIBCPP_ASSERT_UNCATEGORIZED is an undeclared function, resulting in a false positive.

ldionne accepted this revision.Jun 29 2023, 1:02 PM

Ship it! This is great, it's been a long time in the making. Now let's work to bring back some of the important parts that were in there (with an implementation that satisfies the criteria we have now).

This revision is now accepted and ready to land.Jun 29 2023, 1:02 PM
hans added a subscriber: hans.Jul 3 2023, 3:01 AM
hans added inline comments.
libcxx/include/__tree
379–380

This caused a few of Chromium's tests to fail.

Since we set _LIBCPP_ENABLE_ASSERTIONS but not _LIBCPP_ENABLE_DEBUG_MODE, this made std::tree<>::erase O(n) instead of O(log n), causing a few of our tests to time out. (crbug.com/1459922)

It seems this kind of check is too expensive for the regular assert mode. Could this check be disabled for now, or is there some alternative "expensive checks" mode it could be part of?

ldionne added inline comments.Jul 3 2023, 6:36 AM
libcxx/include/__tree
379–380

Interesting -- yes I think this is clearly a debug mode assertion only, we only happen to be in the middle of categorizing those so the current state of the tree doesn't represent what we want to ship in the next release. @var-const I think we could just comment out this assertion if it's causing problems until we categorize it as a debug mode assertion.

alexfh added a subscriber: alexfh.Jul 4 2023, 5:39 AM
alexfh added inline comments.
libcxx/include/__tree
379–380

We're also seeing a number of test timeouts in checked builds after this patch. Thanks Hans for coming up with a fix!

libcxx/src/CMakeLists.txt