Details
- Reviewers
jdoerfert Mordante ldionne - Group Reviewers
Restricted Project - Commits
- rGb5270ba20dc3: [libc++] Remove the legacy debug mode.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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. |
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! |
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 | ||
---|---|---|
53 | Should this be UNCATEGORIZED_ASSERT instead? Assuming you land the patch that does the renaming first, I think it should. | |
libcxx/include/__tree | ||
379–383 | 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 | I don't think this is correct -- this apparently pertains to libstdcxx types (per the comment above). |
libcxx/include/__algorithm/comp_ref_type.h | ||
---|---|---|
53 | 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. |
libcxx/include/__algorithm/comp_ref_type.h | ||
---|---|---|
53 | 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. |
libcxx/include/__algorithm/comp_ref_type.h | ||
---|---|---|
53 | 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. |
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).
libcxx/include/__tree | ||
---|---|---|
379–383 | 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? |
libcxx/include/__tree | ||
---|---|---|
379–383 | 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. |
libcxx/include/__tree | ||
---|---|---|
379–383 | Sent https://reviews.llvm.org/D154417 to do this. |
libcxx/include/__tree | ||
---|---|---|
379–383 | We're also seeing a number of test timeouts in checked builds after this patch. Thanks Hans for coming up with a fix! |
Not attached: We should add a release note.