[base] Define CHECK comparison for signed vs. unsigned

The current CHECK/DCHECK implementation fails statically if a signed
value is compared against an unsigned value. The common solution is to
cast on each caller, which is tedious and error-prone (might hide bugs).
This CL implements signed vs. unsigned comparisons by executing up to
two comparisons. For example, if i is int32_t and u is uint_32_t, a
DCHECK_LE(i, u) would create the check
i <= 0 || static_cast<uint32_t>(i) <= u.
For checks against constants, at least one of the checks can be removed
by compiler optimizations.

The tradeoff we have to make is to sometimes silently execute an
additional comparison. And we increase code complexity of course, even
though the usage is just as easy (or even easier) as before.

The compile time impact seems to be minimal:
I ran 3 full compilations for Optdebug on my local machine, one time on
the current ToT, one time with this CL plus http://crrev.com/2524093002.
Before: 143.72 +- 1.21 seconds
Now: 144.18 +- 0.67 seconds

In order to check that the new comparisons are working, I refactored
some DCHECKs in wasm to use the new magic, and added unit test cases.

R=ishell@chromium.org, titzer@chromium.org
CC=ahaas@chromium.org, bmeurer@chromium.org

Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc
Review-Url: https://codereview.chromium.org/2526783002
Cr-Original-Commit-Position: refs/heads/master@{#41275}
Cr-Commit-Position: refs/heads/master@{#41411}

src/base/logging.h

@@ -111,16 +111,72 @@ DEFINE_MAKE_CHECK_OP_STRING(char const*)
 DEFINE_MAKE_CHECK_OP_STRING(void const*)
 #undef DEFINE_MAKE_CHECK_OP_STRING
 
+// is_signed_vs_unsigned::value is true if both types are integral, Lhs is
+// signed, and Rhs is unsigned. False in all other cases.
+template <typename Lhs, typename Rhs>
+struct is_signed_vs_unsigned {
+  enum : bool {
+    value = std::is_integral<Lhs>::value && std::is_integral<Rhs>::value &&
+            std::is_signed<Lhs>::value && std::is_unsigned<Rhs>::value
+  };
+};
+// Same thing, other way around: Lhs is unsigned, Rhs signed.
+template <typename Lhs, typename Rhs>
+struct is_unsigned_vs_signed : public is_signed_vs_unsigned<Rhs, Lhs> {};
+
+// Specialize the compare functions for signed vs. unsigned comparisons.
+// std::enable_if ensures that this template is only instantiable if both Lhs
+// and Rhs are integral types, and their signedness does not match.
+#define MAKE_UNSIGNED(Type, value) \
+  static_cast<typename std::make_unsigned<Type>::type>(value)
+#define DEFINE_SIGNED_MISMATCH_COMP(CHECK, NAME, IMPL)                  \
+  template <typename Lhs, typename Rhs>                                 \
+  V8_INLINE typename std::enable_if<CHECK<Lhs, Rhs>::value, bool>::type \
+      Cmp##NAME##Impl(Lhs const& lhs, Rhs const& rhs) {                 \
+    return IMPL;                                                        \
+  }
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, EQ,
+                            lhs >= 0 && MAKE_UNSIGNED(Lhs, lhs) == rhs)
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, LT,
+                            lhs < 0 || MAKE_UNSIGNED(Lhs, lhs) < rhs)
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, LE,
+                            lhs <= 0 || MAKE_UNSIGNED(Lhs, lhs) <= rhs)
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, NE, !CmpEQImpl(lhs, rhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, GT, !CmpLEImpl(lhs, rhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, GE, !CmpLTImpl(lhs, rhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, EQ, CmpEQImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, NE, CmpNEImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, LT, CmpGTImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, LE, CmpGEImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, GT, CmpLTImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, GE, CmpLEImpl(rhs, lhs))
+#undef MAKE_UNSIGNED
+#undef DEFINE_SIGNED_MISMATCH_COMP
+
 // Helper functions for CHECK_OP macro.
 // The (float, float) and (double, double) instantiations are explicitly
-// externialized to ensure proper 32/64-bit comparisons on x86.
+// externalized to ensure proper 32/64-bit comparisons on x86.
+// The Cmp##NAME##Impl function is only instantiable if one of the two types is
+// not integral or their signedness matches (i.e. whenever no specialization is
+// required, see above). Otherwise it is disabled by the enable_if construct,
+// and the compiler will pick a specialization from above.
 #define DEFINE_CHECK_OP_IMPL(NAME, op)                                         \
+  template <typename Lhs, typename Rhs>                                        \
+  V8_INLINE                                                                    \
+      typename std::enable_if<!is_signed_vs_unsigned<Lhs, Rhs>::value &&       \
+                                  !is_unsigned_vs_signed<Lhs, Rhs>::value,     \
+                              bool>::type                                      \
+          Cmp##NAME##Impl(typename PassType<Lhs>::type lhs,                    \
+                          typename PassType<Rhs>::type rhs) {                  \
+    return lhs op rhs;                                                         \
+  }                                                                            \
   template <typename Lhs, typename Rhs>                                        \
   V8_INLINE std::string* Check##NAME##Impl(typename PassType<Lhs>::type lhs,   \
                                            typename PassType<Rhs>::type rhs,   \
                                            char const* msg) {                  \
-    return V8_LIKELY(lhs op rhs) ? nullptr                                     \
-                                 : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg); \
+    bool cmp = Cmp##NAME##Impl<Lhs, Rhs>(lhs, rhs);                            \
+    return V8_LIKELY(cmp) ? nullptr                                            \
+                          : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);        \
   }                                                                            \
   extern template V8_BASE_EXPORT std::string* Check##NAME##Impl<float, float>( \
       float lhs, float rhs, char const* msg);                                  \

src/log-utils.h

@@ -110,9 +110,9 @@ class Log {
 
   // Implementation of writing to a log file.
   int WriteToFile(const char* msg, int length) {
-    DCHECK(output_handle_ != NULL);
+    DCHECK_NOT_NULL(output_handle_);
     size_t rv = fwrite(msg, 1, length, output_handle_);
-    DCHECK(static_cast<size_t>(length) == rv);
+    DCHECK_EQ(length, rv);
     USE(rv);
     fflush(output_handle_);
     return length;

src/wasm/ast-decoder.cc

@@ -88,7 +88,7 @@ struct MergeValues {
   } vals;  // Either multiple values or a single value.
 
   Value& first() {
-    DCHECK_GT(arity, 0u);
+    DCHECK_GT(arity, 0);
     return arity == 1 ? vals.first : vals.array[0];
   }
 };
@@ -577,7 +577,7 @@ class WasmFullDecoder : public WasmDecoder {
 
   // Decodes the locals declarations, if any, populating {local_type_vec_}.
   void DecodeLocalDecls() {
-    DCHECK_EQ(0u, local_type_vec_.size());
+    DCHECK_EQ(0, local_type_vec_.size());
     // Initialize {local_type_vec} from signature.
     if (sig_) {
       local_type_vec_.reserve(sig_->parameter_count());
@@ -1443,7 +1443,7 @@ class WasmFullDecoder : public WasmDecoder {
       Value val = {pc_, nullptr, kAstStmt};
       return val;
     } else {
-      DCHECK_LE(stack_depth, static_cast<int>(stack_.size()));
+      DCHECK_LE(stack_depth, stack_.size());
       Value val = Pop();
       stack_.resize(stack_depth);
       return val;

src/wasm/wasm-interpreter.cc

@@ -1171,7 +1171,7 @@ class ThreadImpl : public WasmInterpreter::Thread {
   }
 
   bool DoReturn(InterpreterCode** code, pc_t* pc, pc_t* limit, size_t arity) {
-    DCHECK_GT(frames_.size(), 0u);
+    DCHECK_GT(frames_.size(), 0);
     // Pop all blocks for this frame.
     while (!blocks_.empty() && blocks_.back().fp == frames_.size()) {
       blocks_.pop_back();
@@ -1678,8 +1678,8 @@ class ThreadImpl : public WasmInterpreter::Thread {
   }
 
   WasmVal Pop() {
-    DCHECK_GT(stack_.size(), 0u);
-    DCHECK_GT(frames_.size(), 0u);
+    DCHECK_GT(stack_.size(), 0);
+    DCHECK_GT(frames_.size(), 0);
     DCHECK_GT(stack_.size(), frames_.back().llimit());  // can't pop into locals
     WasmVal val = stack_.back();
     stack_.pop_back();
@@ -1687,8 +1687,8 @@ class ThreadImpl : public WasmInterpreter::Thread {
   }
 
   void PopN(int n) {
-    DCHECK_GE(stack_.size(), static_cast<size_t>(n));
-    DCHECK_GT(frames_.size(), 0u);
+    DCHECK_GE(stack_.size(), n);
+    DCHECK_GT(frames_.size(), 0);
     size_t nsize = stack_.size() - n;
     DCHECK_GE(nsize, frames_.back().llimit());  // can't pop into locals
     stack_.resize(nsize);
@@ -1696,7 +1696,7 @@ class ThreadImpl : public WasmInterpreter::Thread {
 
   WasmVal PopArity(size_t arity) {
     if (arity == 0) return WasmVal();
-    CHECK_EQ(1u, arity);
+    CHECK_EQ(1, arity);
     return Pop();
   }
 

src/wasm/wasm-module.cc

@@ -52,8 +52,8 @@ MaybeHandle<String> ExtractStringFromModuleBytes(
     uint32_t offset, uint32_t size) {
   // TODO(wasm): cache strings from modules if it's a performance win.
   Handle<SeqOneByteString> module_bytes = compiled_module->module_bytes();
-  DCHECK_GE(static_cast<size_t>(module_bytes->length()), offset);
-  DCHECK_GE(static_cast<size_t>(module_bytes->length() - offset), size);
+  DCHECK_GE(module_bytes->length(), offset);
+  DCHECK_GE(module_bytes->length() - offset, size);
   Address raw = module_bytes->GetCharsAddress() + offset;
   if (!unibrow::Utf8::Validate(reinterpret_cast<const byte*>(raw), size))
     return {};  // UTF8 decoding error for name.
@@ -111,7 +111,7 @@ void* TryAllocateBackingStore(Isolate* isolate, size_t size,
     // addressable memory after the guard page can be made inaccessible.
     const size_t alloc_size =
         RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize());
-    DCHECK_EQ(0u, size % base::OS::CommitPageSize());
+    DCHECK_EQ(0, size % base::OS::CommitPageSize());
 
     // AllocateGuarded makes the whole region inaccessible by default.
     void* memory = base::OS::AllocateGuarded(alloc_size);
@@ -2074,7 +2074,7 @@ MaybeHandle<WasmModuleObject> wasm::CreateModuleObjectFromBytes(
     compiled_module->set_script(asm_js_script);
     size_t offset_table_len =
         asm_js_offset_tables_end - asm_js_offset_tables_start;
-    DCHECK_GE(static_cast<size_t>(kMaxInt), offset_table_len);
+    DCHECK_GE(kMaxInt, offset_table_len);
     Handle<ByteArray> offset_table =
         isolate->factory()->NewByteArray(static_cast<int>(offset_table_len));
     memcpy(offset_table->GetDataStartAddress(), asm_js_offset_tables_start,

src/wasm/wasm-module.h

@@ -277,7 +277,7 @@ struct V8_EXPORT_PRIVATE ModuleWireBytes {
   WasmName GetName(uint32_t offset, uint32_t length) const {
     if (length == 0) return {"<?>", 3};  // no name.
     CHECK(BoundsCheck(offset, length));
-    DCHECK_GE(static_cast<int>(length), 0);
+    DCHECK_GE(length, 0);
     return Vector<const char>::cast(
         module_bytes.SubVector(offset, offset + length));
   }
@@ -291,7 +291,7 @@ struct V8_EXPORT_PRIVATE ModuleWireBytes {
   WasmName GetNameOrNull(uint32_t offset, uint32_t length) const {
     if (offset == 0 && length == 0) return {NULL, 0};  // no name.
     CHECK(BoundsCheck(offset, length));
-    DCHECK_GE(static_cast<int>(length), 0);
+    DCHECK_GE(length, 0);
     return Vector<const char>::cast(
         module_bytes.SubVector(offset, offset + length));
   }

src/wasm/wasm-objects.cc

@@ -51,7 +51,7 @@ static uint32_t SafeUint32(Object* value) {
   DCHECK(value->IsHeapNumber());
   HeapNumber* num = HeapNumber::cast(value);
   CHECK_GE(num->value(), 0.0);
-  CHECK_LE(num->value(), static_cast<double>(kMaxUInt32));
+  CHECK_LE(num->value(), kMaxUInt32);
   return static_cast<uint32_t>(num->value());
 }
 
@@ -61,8 +61,8 @@ static int32_t SafeInt32(Object* value) {
   }
   DCHECK(value->IsHeapNumber());
   HeapNumber* num = HeapNumber::cast(value);
-  CHECK_GE(num->value(), static_cast<double>(Smi::kMinValue));
-  CHECK_LE(num->value(), static_cast<double>(Smi::kMaxValue));
+  CHECK_GE(num->value(), Smi::kMinValue);
+  CHECK_LE(num->value(), Smi::kMaxValue);
   return static_cast<int32_t>(num->value());
 }
 
@@ -394,9 +394,8 @@ Vector<const uint8_t> WasmCompiledModule::GetRawFunctionName(
   DCHECK_GT(module()->functions.size(), func_index);
   WasmFunction& function = module()->functions[func_index];
   SeqOneByteString* bytes = ptr_to_module_bytes();
-  DCHECK_GE(static_cast<size_t>(bytes->length()), function.name_offset);
-  DCHECK_GE(static_cast<size_t>(bytes->length() - function.name_offset),
-            function.name_length);
+  DCHECK_GE(bytes->length(), function.name_offset);
+  DCHECK_GE(bytes->length() - function.name_offset, function.name_length);
   return Vector<const uint8_t>(bytes->GetCharsAddress() + function.name_offset,
                                function.name_length);
 }
@@ -404,8 +403,7 @@ Vector<const uint8_t> WasmCompiledModule::GetRawFunctionName(
 int WasmCompiledModule::GetFunctionOffset(uint32_t func_index) const {
   std::vector<WasmFunction>& functions = module()->functions;
   if (static_cast<uint32_t>(func_index) >= functions.size()) return -1;
-  DCHECK_GE(static_cast<uint32_t>(kMaxInt),
-            functions[func_index].code_start_offset);
+  DCHECK_GE(kMaxInt, functions[func_index].code_start_offset);
   return static_cast<int>(functions[func_index].code_start_offset);
 }
 
@@ -470,7 +468,7 @@ Handle<ByteArray> GetDecodedAsmJsOffsetTable(
   }
   // Wasm bytes must be valid and must contain asm.js offset table.
   DCHECK(asm_offsets.ok());
-  DCHECK_GE(static_cast<size_t>(kMaxInt), asm_offsets.val.size());
+  DCHECK_GE(kMaxInt, asm_offsets.val.size());
   int num_functions = static_cast<int>(asm_offsets.val.size());
   int num_imported_functions =
       static_cast<int>(compiled_module->module()->num_imported_functions);

src/wasm/wasm-text.cc

@@ -232,7 +232,7 @@ void wasm::PrintWasmText(
       }
       case kExprCallIndirect: {
         CallIndirectOperand operand(&i, i.pc());
-        DCHECK_EQ(0U, operand.table_index);
+        DCHECK_EQ(0, operand.table_index);
         os << "call_indirect " << operand.index;
         break;
       }

test/unittests/base/logging-unittest.cc

@@ -2,24 +2,64 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <cstdint>
+
 #include "src/base/logging.h"
+#include "src/objects.h"
 #include "testing/gtest-support.h"
 
 namespace v8 {
 namespace base {
 
 namespace {
-template <typename Lhs, typename Rhs>
-std::string *CallCheckEQ(Lhs lhs, Rhs rhs, const char *msg) {
-  return CheckEQImpl<Lhs, Rhs>(lhs, rhs, msg);
-}
+
+#define CHECK_SUCCEED(NAME, lhs, rhs)                                      \
+  {                                                                        \
+    std::string* error_message =                                           \
+        Check##NAME##Impl<decltype(lhs), decltype(rhs)>((lhs), (rhs), ""); \
+    EXPECT_EQ(nullptr, error_message);                                     \
+  }
+
+#define CHECK_FAIL(NAME, lhs, rhs)                                         \
+  {                                                                        \
+    std::string* error_message =                                           \
+        Check##NAME##Impl<decltype(lhs), decltype(rhs)>((lhs), (rhs), ""); \
+    EXPECT_NE(nullptr, error_message);                                     \
+  }
+
 }  // namespace
 
 TEST(LoggingTest, CheckEQImpl) {
-  EXPECT_EQ(nullptr, CallCheckEQ(0.0, 0.0, ""));
-  EXPECT_EQ(nullptr, CallCheckEQ(0.0, -0.0, ""));
-  EXPECT_EQ(nullptr, CallCheckEQ(-0.0, 0.0, ""));
-  EXPECT_EQ(nullptr, CallCheckEQ(-0.0, -0.0, ""));
+  CHECK_SUCCEED(EQ, 0.0, 0.0)
+  CHECK_SUCCEED(EQ, 0.0, -0.0)
+  CHECK_SUCCEED(EQ, -0.0, 0.0)
+  CHECK_SUCCEED(EQ, -0.0, -0.0)
+}
+
+TEST(LoggingTest, CompareSignedMismatch) {
+  CHECK_SUCCEED(EQ, static_cast<int32_t>(14), static_cast<uint32_t>(14))
+  CHECK_FAIL(EQ, static_cast<int32_t>(14), static_cast<uint32_t>(15))
+  CHECK_FAIL(EQ, static_cast<int32_t>(-1), static_cast<uint32_t>(-1))
+  CHECK_SUCCEED(LT, static_cast<int32_t>(-1), static_cast<uint32_t>(0))
+  CHECK_SUCCEED(LT, static_cast<int32_t>(-1), static_cast<uint32_t>(-1))
+  CHECK_SUCCEED(LE, static_cast<int32_t>(-1), static_cast<uint32_t>(0))
+  CHECK_SUCCEED(LE, static_cast<int32_t>(55), static_cast<uint32_t>(55))
+  CHECK_SUCCEED(LT, static_cast<int32_t>(55), static_cast<uint32_t>(0x7fffff00))
+  CHECK_SUCCEED(LE, static_cast<int32_t>(55), static_cast<uint32_t>(0x7fffff00))
+  CHECK_SUCCEED(GE, static_cast<uint32_t>(0x7fffff00), static_cast<int32_t>(55))
+  CHECK_SUCCEED(GT, static_cast<uint32_t>(0x7fffff00), static_cast<int32_t>(55))
+  CHECK_SUCCEED(GT, static_cast<uint32_t>(-1), static_cast<int32_t>(-1))
+  CHECK_SUCCEED(GE, static_cast<uint32_t>(0), static_cast<int32_t>(-1))
+  CHECK_SUCCEED(LT, static_cast<int8_t>(-1), static_cast<uint32_t>(0))
+  CHECK_SUCCEED(GT, static_cast<uint64_t>(0x7f01010101010101), 0)
+  CHECK_SUCCEED(LE, static_cast<int64_t>(0xff01010101010101),
+                static_cast<uint8_t>(13))
+}
+
+TEST(LoggingTest, CompareAgainstStaticConstPointer) {
+  // These used to produce link errors before http://crrev.com/2524093002.
+  CHECK_FAIL(EQ, v8::internal::Smi::kZero, v8::internal::Smi::FromInt(17));
+  CHECK_SUCCEED(GT, 0, v8::internal::Smi::kMinValue);
 }
 
 }  // namespace base