From e7a93c149c88474714adc758c91e973dd7662509 Mon Sep 17 00:00:00 2001 From: "J. Gonzalez" Date: Sun, 20 Sep 2020 15:09:06 -0400 Subject: [PATCH] Toolchain related fixes (#1192) * Fix gcc-10 uint32_t related issues gcc-10 doesn't like how we didn't include cstdint in one file; in other cases, it didn't like the implicit cast to an unsigned. In another case, it didn't like that we return a uint64_t as an element of a uint32_t. In the last case, we now limit the amount of rows we can affect or return to 2^32. This is technically a functional change, but before, returning 4 billion rows would've caused issues at the network layer, so it isn't a *real* functional change. * Fix gcc-10 -Wclass-memaccess issue Recasts to a void* before using memset in order to appease -Wclass-memaccess. * spdlog bugfix See b962fbb15c9f1fb2f5410ff0cf3a4065088c717e * json library bugfix See c61a9071ae9410daa629246c46a24165626f992b * Use references instead of copies clang-10 bugfix * Obscure numeric representability bugfix Would like someone to review this to make sure I didn't hallucinate anything. Fixes clang-10 warnings. * clang-format fixes oop --- src/binder/binder_util.cpp | 8 +++++- src/catalog/database_catalog.cpp | 4 +-- .../common/container/concurrent_bitmap.h | 3 ++- .../execution/exec/execution_context.h | 4 +-- src/include/execution/exec/output.h | 4 +-- .../execution/sql/operators/cast_operators.h | 8 +++++- src/include/storage/access_observer.h | 1 + src/traffic_cop/traffic_cop.cpp | 24 +++++++++--------- .../nlohmann/detail/output/serializer.hpp | 2 +- third_party/spdlog/common.h | 25 +++++++------------ 10 files changed, 45 insertions(+), 38 deletions(-) diff --git a/src/binder/binder_util.cpp b/src/binder/binder_util.cpp index 337bbf6622..1acca7d464 100644 --- a/src/binder/binder_util.cpp +++ b/src/binder/binder_util.cpp @@ -182,7 +182,13 @@ void BinderUtil::CheckAndTryPromoteType(const common::ManagedPointer bool BinderUtil::IsRepresentable(const Input int_val) { - return std::numeric_limits::lowest() <= int_val && int_val <= std::numeric_limits::max(); + // Fixes this hideously obscure bug: https://godbolt.org/z/M14jdb + if constexpr (std::numeric_limits::is_integer && !std::numeric_limits::is_integer) { // NOLINT + return std::numeric_limits::lowest() <= int_val && + static_cast(int_val) < std::numeric_limits::max(); + } else { // NOLINT + return std::numeric_limits::lowest() <= int_val && int_val <= std::numeric_limits::max(); + } } /** diff --git a/src/catalog/database_catalog.cpp b/src/catalog/database_catalog.cpp index f3a4f2e2d5..057f2622c5 100644 --- a/src/catalog/database_catalog.cpp +++ b/src/catalog/database_catalog.cpp @@ -444,7 +444,7 @@ bool DatabaseCatalog::DeleteNamespace(const common::ManagedPointer(object.first)); @@ -461,7 +461,7 @@ bool DatabaseCatalog::DeleteNamespace(const common::ManagedPointer(bits_), 0, size); } // TODO(Tianyu): We will eventually need optimization for bulk checks and diff --git a/src/include/execution/exec/execution_context.h b/src/include/execution/exec/execution_context.h index 18acc5fa21..240bd56a38 100644 --- a/src/include/execution/exec/execution_context.h +++ b/src/include/execution/exec/execution_context.h @@ -177,7 +177,7 @@ class EXPORT ExecutionContext { * INSERT, UPDATE, and DELETE queries return a number for the rows affected, so this should be incremented in the root * nodes of the query */ - uint64_t &RowsAffected() { return rows_affected_; } + uint32_t &RowsAffected() { return rows_affected_; } /** * Set the PipelineOperatingUnits @@ -219,7 +219,7 @@ class EXPORT ExecutionContext { common::ManagedPointer accessor_; common::ManagedPointer> params_; uint8_t execution_mode_; - uint64_t rows_affected_ = 0; + uint32_t rows_affected_ = 0; bool memory_use_override_ = false; uint32_t memory_use_override_value_ = 0; diff --git a/src/include/execution/exec/output.h b/src/include/execution/exec/output.h index 1181e43aba..6a7c0a7d48 100644 --- a/src/include/execution/exec/output.h +++ b/src/include/execution/exec/output.h @@ -139,10 +139,10 @@ class OutputWriter { /** * @return number of rows printed */ - uint64_t NumRows() const { return num_rows_; } + uint32_t NumRows() const { return num_rows_; } private: - uint64_t num_rows_ = 0; + uint32_t num_rows_ = 0; const common::ManagedPointer schema_; const common::ManagedPointer out_; const std::vector &field_formats_; diff --git a/src/include/execution/sql/operators/cast_operators.h b/src/include/execution/sql/operators/cast_operators.h index a773d31418..0d5afe2b86 100644 --- a/src/include/execution/sql/operators/cast_operators.h +++ b/src/include/execution/sql/operators/cast_operators.h @@ -243,7 +243,13 @@ struct EXPORT TryCast< constexpr OutType k_max = std::numeric_limits::max(); *output = static_cast(input); - return input >= k_min && input <= k_max; + + // Fixes this hideously obscure bug: https://godbolt.org/z/M14jdb + if constexpr (std::numeric_limits::is_integer && !std::numeric_limits::is_integer) { // NOLINT + return k_min <= input && static_cast(input) < k_max; + } else { // NOLINT + return k_min <= input && input <= k_max; + } } }; diff --git a/src/include/storage/access_observer.h b/src/include/storage/access_observer.h index b435450f54..d98cd8ee06 100644 --- a/src/include/storage/access_observer.h +++ b/src/include/storage/access_observer.h @@ -1,5 +1,6 @@ #pragma once +#include #include namespace terrier::storage { diff --git a/src/traffic_cop/traffic_cop.cpp b/src/traffic_cop/traffic_cop.cpp index c1295a120a..8512e215d4 100644 --- a/src/traffic_cop/traffic_cop.cpp +++ b/src/traffic_cop/traffic_cop.cpp @@ -170,7 +170,7 @@ TrafficCopResult TrafficCop::ExecuteSetStatement(common::ManagedPointer(), connection_ctx->Accessor(), connection_ctx->GetDatabaseOid())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } case network::QueryType::QUERY_CREATE_DB: { if (execution::sql::DDLExecutors::CreateDatabaseExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } case network::QueryType::QUERY_CREATE_INDEX: { if (execution::sql::DDLExecutors::CreateIndexExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } case network::QueryType::QUERY_CREATE_SCHEMA: { if (execution::sql::DDLExecutors::CreateNamespaceExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } @@ -242,7 +242,7 @@ TrafficCopResult TrafficCop::ExecuteDropStatement( case network::QueryType::QUERY_DROP_TABLE: { if (execution::sql::DDLExecutors::DropTableExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } @@ -250,21 +250,21 @@ TrafficCopResult TrafficCop::ExecuteDropStatement( if (execution::sql::DDLExecutors::DropDatabaseExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor(), connection_ctx->GetDatabaseOid())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } case network::QueryType::QUERY_DROP_INDEX: { if (execution::sql::DDLExecutors::DropIndexExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } case network::QueryType::QUERY_DROP_SCHEMA: { if (execution::sql::DDLExecutors::DropNamespaceExecutor( physical_plan.CastManagedPointerTo(), connection_ctx->Accessor())) { - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } break; } @@ -335,7 +335,7 @@ TrafficCopResult TrafficCop::BindQuery( return {ResultType::ERROR, error}; } - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } TrafficCopResult TrafficCop::CodegenPhysicalPlan( @@ -353,7 +353,7 @@ TrafficCopResult TrafficCop::CodegenPhysicalPlan( if (portal->GetStatement()->GetExecutableQuery() != nullptr && use_query_cache_) { // We've already codegen'd this, move on... - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } // TODO(WAN): see #1047 @@ -366,7 +366,7 @@ TrafficCopResult TrafficCop::CodegenPhysicalPlan( // TODO(Matt): handle code generation failing portal->GetStatement()->SetExecutableQuery(std::move(exec_query)); - return {ResultType::COMPLETE, 0}; + return {ResultType::COMPLETE, 0u}; } TrafficCopResult TrafficCop::RunExecutableQuery(const common::ManagedPointer connection_ctx, diff --git a/third_party/nlohmann/detail/output/serializer.hpp b/third_party/nlohmann/detail/output/serializer.hpp index a644264449..50dded01ab 100644 --- a/third_party/nlohmann/detail/output/serializer.hpp +++ b/third_party/nlohmann/detail/output/serializer.hpp @@ -445,7 +445,7 @@ class serializer return; } - const bool is_negative = (x <= 0) and (x != 0); // see issue #755 + const bool is_negative = not (x >= 0); std::size_t i = 0; while (x != 0) diff --git a/third_party/spdlog/common.h b/third_party/spdlog/common.h index b8d45175db..be3349086c 100644 --- a/third_party/spdlog/common.h +++ b/third_party/spdlog/common.h @@ -131,35 +131,28 @@ enum class pattern_time_type // // Log exception // -class spdlog_ex : public std::runtime_error +class spdlog_ex : public std::exception { public: explicit spdlog_ex(const std::string &msg) - : runtime_error(msg) + : msg_(msg) { } + spdlog_ex(std::string msg, int last_errno) - : runtime_error(std::move(msg)) - , last_errno_(last_errno) { + fmt::memory_buffer outbuf; + fmt::format_system_error(outbuf, last_errno, msg); + msg_ = fmt::to_string(outbuf); } + const char *what() const SPDLOG_NOEXCEPT override { - if (last_errno_) - { - fmt::memory_buffer buf; - std::string msg(runtime_error::what()); - fmt::format_system_error(buf, last_errno_, msg); - return fmt::to_string(buf).c_str(); - } - else - { - return runtime_error::what(); - } + return msg_.c_str(); } private: - int last_errno_{0}; + std::string msg_; }; //