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

@@ -182,7 +182,13 @@ void BinderUtil::CheckAndTryPromoteType(const common::ManagedPointer<parser::Con
 
 template <typename Output, typename Input>
 bool BinderUtil::IsRepresentable(const Input int_val) {
-  return std::numeric_limits<Output>::lowest() <= int_val && int_val <= std::numeric_limits<Output>::max();
+  // Fixes this hideously obscure bug: https://godbolt.org/z/M14jdb
+  if constexpr (std::numeric_limits<Output>::is_integer && !std::numeric_limits<Input>::is_integer) {  // NOLINT
+    return std::numeric_limits<Output>::lowest() <= int_val &&
+           static_cast<Output>(int_val) < std::numeric_limits<Output>::max();
+  } else {  // NOLINT
+    return std::numeric_limits<Output>::lowest() <= int_val && int_val <= std::numeric_limits<Output>::max();
+  }
 }
 
 /**

src/catalog/database_catalog.cpp

@@ -444,7 +444,7 @@ bool DatabaseCatalog::DeleteNamespace(const common::ManagedPointer<transaction::
   // Step 4: Cascading deletes
   // Get the objects in this namespace
   auto ns_objects = GetNamespaceClassOids(txn, ns_oid);
-  for (const auto object : ns_objects) {
+  for (const auto &object : ns_objects) {
     // Delete all of the tables. This should get most of the indexes
     if (object.second == postgres::ClassKind::REGULAR_TABLE) {
       result = DeleteTable(txn, static_cast<table_oid_t>(object.first));
@@ -461,7 +461,7 @@ bool DatabaseCatalog::DeleteNamespace(const common::ManagedPointer<transaction::
   // advantage of existing PRIs and indexes and expecting that deleting a namespace isn't that common of an operation,
   // so we can be slightly less efficient than optimal.
   ns_objects = GetNamespaceClassOids(txn, ns_oid);
-  for (const auto object : ns_objects) {
+  for (const auto &object : ns_objects) {
     // Delete all of the straggler indexes that may have been built on tables in other namespaces. We shouldn't get any
     // double-deletions because indexes on tables will already be invisible to us (logically deleted already).
     if (object.second == postgres::ClassKind::INDEX) {

src/include/common/container/concurrent_bitmap.h

@@ -169,7 +169,8 @@ class RawConcurrentBitmap {
    */
   void UnsafeClear(const uint32_t num_bits) {
     auto size = RawBitmap::SizeInBytes(num_bits);
-    std::memset(bits_, 0, size);
+    // Recast bits_ as workaround for -Wclass-memaccess
+    std::memset(static_cast<void *>(bits_), 0, size);
   }
 
   // TODO(Tianyu): We will eventually need optimization for bulk checks and

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<catalog::CatalogAccessor> accessor_;
   common::ManagedPointer<const std::vector<parser::ConstantValueExpression>> 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;

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<planner::OutputSchema> schema_;
   const common::ManagedPointer<network::PostgresPacketWriter> out_;
   const std::vector<network::FieldFormat> &field_formats_;

src/include/execution/sql/operators/cast_operators.h

@@ -243,7 +243,13 @@ struct EXPORT TryCast<
     constexpr OutType k_max = std::numeric_limits<OutType>::max();
 
     *output = static_cast<OutType>(input);
-    return input >= k_min && input <= k_max;
+
+    // Fixes this hideously obscure bug: https://godbolt.org/z/M14jdb
+    if constexpr (std::numeric_limits<OutType>::is_integer && !std::numeric_limits<InType>::is_integer) {  // NOLINT
+      return k_min <= input && static_cast<OutType>(input) < k_max;
+    } else {  // NOLINT
+      return k_min <= input && input <= k_max;
+    }
   }
 };
 

src/include/storage/access_observer.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <cstdint>
 #include <unordered_map>
 
 namespace terrier::storage {

src/traffic_cop/traffic_cop.cpp

@@ -170,7 +170,7 @@ TrafficCopResult TrafficCop::ExecuteSetStatement(common::ManagedPointer<network:
     return {ResultType::ERROR, error};
   }
 
-  return {ResultType::COMPLETE, 0};
+  return {ResultType::COMPLETE, 0u};
 }
 
 TrafficCopResult TrafficCop::ExecuteCreateStatement(
@@ -189,28 +189,28 @@ TrafficCopResult TrafficCop::ExecuteCreateStatement(
       if (execution::sql::DDLExecutors::CreateTableExecutor(
               physical_plan.CastManagedPointerTo<planner::CreateTablePlanNode>(), 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<planner::CreateDatabasePlanNode>(), 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<planner::CreateIndexPlanNode>(), 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<planner::CreateNamespacePlanNode>(), connection_ctx->Accessor())) {
-        return {ResultType::COMPLETE, 0};
+        return {ResultType::COMPLETE, 0u};
       }
       break;
     }
@@ -242,29 +242,29 @@ TrafficCopResult TrafficCop::ExecuteDropStatement(
     case network::QueryType::QUERY_DROP_TABLE: {
       if (execution::sql::DDLExecutors::DropTableExecutor(
               physical_plan.CastManagedPointerTo<planner::DropTablePlanNode>(), connection_ctx->Accessor())) {
-        return {ResultType::COMPLETE, 0};
+        return {ResultType::COMPLETE, 0u};
       }
       break;
     }
     case network::QueryType::QUERY_DROP_DB: {
       if (execution::sql::DDLExecutors::DropDatabaseExecutor(
               physical_plan.CastManagedPointerTo<planner::DropDatabasePlanNode>(), 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<planner::DropIndexPlanNode>(), 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<planner::DropNamespacePlanNode>(), 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<network::ConnectionContext> connection_ctx,

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)

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_;
 };
 
 //