From b9b7a1091bc13dfc3c5fe7528432dbc5367958b0 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Mon, 30 Sep 2024 16:58:33 +0000 Subject: [PATCH] (Re)-enable libcxx debug mode - We previously compiled libcxx with _LIBCPP_DEBUG=0. In old libcxx versions, this (surprisingly) enabled the basic debug mode [1]. - In libcxx 15 (the version we are currently using), _LIBCPP_DEBUG=0 does nothing [2], the replacement is _LIBCPP_ENABLE_DEBUG_MODE=1. - The debug mode is only enabled in Debug builds. Their docs say the extra check change complexity guarantees + the asserts crash which we don't want in Release builds. The debug mode detects issues like in [3]. Crashes look like this (for example) ``` /data/ch/contrib/llvm-project/libcxx/include/__iterator/wrap_iter.h:99: assertion ::std::__libcpp_is_constant_evaluated() || (__get_const_db()->__dereferenceable(this)) failed: Attempted to increment a non-incrementable iteratorAborted (core dumped) ``` - I had to mute some new clang-tidy warnings in places that deal with container iterators. They got heavier and copy-by-value now yields a warning, e.g. for (auto it : iterators) /// <-- warning [...] [1] https://releases.llvm.org/12.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html [2] https://releases.llvm.org/15.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html [3] https://github.com/llvm/llvm-project/blob/main/libcxx/test/support/container_debug_tests.h --- cmake/cxx.cmake | 19 ++++++++++++++++++- src/Common/DNSResolver.cpp | 2 +- .../proxyConfigurationToPocoProxyConfig.cpp | 12 ++++-------- src/DataTypes/EnumValues.h | 6 +++--- src/Storages/MaterializedView/RefreshSet.cpp | 2 +- src/Storages/MergeTree/MergeTask.cpp | 2 +- src/Storages/StorageFuzzJSON.cpp | 1 - 7 files changed, 28 insertions(+), 16 deletions(-) diff --git a/cmake/cxx.cmake b/cmake/cxx.cmake index 7d93bf05fc7..afa41a66129 100644 --- a/cmake/cxx.cmake +++ b/cmake/cxx.cmake @@ -1,4 +1,21 @@ -set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -D_LIBCPP_DEBUG=0") # More checks in debug build. +if (CMAKE_BUILD_TYPE_UC STREQUAL "DEBUG") + # Enable libcxx debug mode: + # --> https://releases.llvm.org/15.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html + # The docs say the debug mode violates complexity guarantees, so do this only for Debug builds. + set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -D_LIBCPP_ENABLE_DEBUG_MODE=1") + + # Also, enable randomization of unspecified behavior, e.g. the order of equal elements in std::sort. + # "Randomization" means that Debug and Release builds use different seeds. + set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -D_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY_SEED=12345678") + + # Note: Libcxx also has provides extra assertions: + # --> https://releases.llvm.org/15.0.0/projects/libcxx/docs/UsingLibcxx.html#assertions-mode + # These look orthogonal to the debug mode but the debug mode imlicitly enables them: + # --> https://github.com/llvm/llvm-project/blob/release/15.x/libcxx/include/__assert#L29 + + # TODO Once we upgrade to LLVM 18+, reconsider all of the above as they introduced "hardening modes": + # https://libcxx.llvm.org/Hardening.html +endif () add_subdirectory(contrib/libcxxabi-cmake) add_subdirectory(contrib/libcxx-cmake) diff --git a/src/Common/DNSResolver.cpp b/src/Common/DNSResolver.cpp index 5a18ef893e0..4fad27fe7d7 100644 --- a/src/Common/DNSResolver.cpp +++ b/src/Common/DNSResolver.cpp @@ -428,7 +428,7 @@ bool DNSResolver::updateCacheImpl( { updated = true; String deleted_elements; - for (auto it : elements_to_drop) + for (auto & it : elements_to_drop) { if (!deleted_elements.empty()) deleted_elements += ", "; diff --git a/src/Common/proxyConfigurationToPocoProxyConfig.cpp b/src/Common/proxyConfigurationToPocoProxyConfig.cpp index c06014ac2dc..ed4742aaaae 100644 --- a/src/Common/proxyConfigurationToPocoProxyConfig.cpp +++ b/src/Common/proxyConfigurationToPocoProxyConfig.cpp @@ -25,15 +25,11 @@ namespace * `curl` strips leading dot and accepts url gitlab.com as a match for no_proxy .gitlab.com, * while `wget` does an exact match. * */ -std::string buildPocoRegexpEntryWithoutLeadingDot(const std::string & host) +std::string buildPocoRegexpEntryWithoutLeadingDot(std::string_view host) { - std::string_view view_without_leading_dot = host; - if (host[0] == '.') - { - view_without_leading_dot = std::string_view {host.begin() + 1u, host.end()}; - } - - return RE2::QuoteMeta(view_without_leading_dot); + if (host.starts_with('.')) + host.remove_prefix(1); + return RE2::QuoteMeta(host); } } diff --git a/src/DataTypes/EnumValues.h b/src/DataTypes/EnumValues.h index 161ca2425c3..ec5991abff1 100644 --- a/src/DataTypes/EnumValues.h +++ b/src/DataTypes/EnumValues.h @@ -36,8 +36,8 @@ public: auto findByValue(const T & value) const { - const auto it = value_to_name_map.find(value); - if (it == std::end(value_to_name_map)) + auto it = value_to_name_map.find(value); + if (it == value_to_name_map.end()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Unexpected value {} in enum", toString(value)); return it; @@ -58,7 +58,7 @@ public: bool getNameForValue(const T & value, StringRef & result) const { const auto it = value_to_name_map.find(value); - if (it == std::end(value_to_name_map)) + if (it == value_to_name_map.end()) return false; result = it->second; diff --git a/src/Storages/MaterializedView/RefreshSet.cpp b/src/Storages/MaterializedView/RefreshSet.cpp index f1000cfd3d9..ddcdc0a8697 100644 --- a/src/Storages/MaterializedView/RefreshSet.cpp +++ b/src/Storages/MaterializedView/RefreshSet.cpp @@ -23,7 +23,7 @@ RefreshSet::Handle & RefreshSet::Handle::operator=(Handle && other) noexcept id = std::move(other.id); inner_table_id = std::move(other.inner_table_id); dependencies = std::move(other.dependencies); - iter = std::move(other.iter); + iter = std::move(other.iter); /// NOLINT(performance-move-const-arg) inner_table_iter = std::move(other.inner_table_iter); metric_increment = std::move(other.metric_increment); return *this; diff --git a/src/Storages/MergeTree/MergeTask.cpp b/src/Storages/MergeTree/MergeTask.cpp index 9c37f205174..eb4ccbdedd4 100644 --- a/src/Storages/MergeTree/MergeTask.cpp +++ b/src/Storages/MergeTree/MergeTask.cpp @@ -604,7 +604,7 @@ MergeTask::StageRuntimeContextPtr MergeTask::ExecuteAndFinalizeHorizontalPart::g new_ctx->rows_sources_temporary_file = std::move(ctx->rows_sources_temporary_file); new_ctx->column_sizes = std::move(ctx->column_sizes); new_ctx->compression_codec = std::move(ctx->compression_codec); - new_ctx->it_name_and_type = std::move(ctx->it_name_and_type); + new_ctx->it_name_and_type = std::move(ctx->it_name_and_type); /// NOLINT(performance-move-const-arg) new_ctx->read_with_direct_io = std::move(ctx->read_with_direct_io); new_ctx->need_sync = std::move(ctx->need_sync); diff --git a/src/Storages/StorageFuzzJSON.cpp b/src/Storages/StorageFuzzJSON.cpp index 4eb8ca88223..fcd6006a6f9 100644 --- a/src/Storages/StorageFuzzJSON.cpp +++ b/src/Storages/StorageFuzzJSON.cpp @@ -149,7 +149,6 @@ void traverse(const ParserImpl::Element & e, std::shared_ptr node) std::shared_ptr parseJSON(const String & json) { - std::string_view view{json.begin(), json.end()}; ParserImpl::Element document; ParserImpl p;