Revert of [base] Define CHECK comparison for signed vs. unsigned (patchset #5...

Revert of [base] Define CHECK comparison for signed vs. unsigned (patchset #5 id:80001 of https://codereview.chromium.org/2526783002/ )

Reason for revert:
Need to revert previous CL because of Android compile error, and this one depends in it.

Original issue's description:
> [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.
>
> R=bmeurer@chromium.org, titzer@chromium.org
>
> Committed: https://crrev.com/5925074a9dab5a8577766545b91b62f2c531d3dc
> Cr-Commit-Position: refs/heads/master@{#41275}

TBR=ishell@chromium.org,titzer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2531533003
Cr-Commit-Position: refs/heads/master@{#41277}

src/base/logging.h

@@ -115,77 +115,21 @@ 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
-// 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.
+// externialized to ensure proper 32/64-bit comparisons on x86.
 #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) {                  \
-    bool cmp = Cmp##NAME##Impl<Lhs, Rhs>(lhs, rhs);                            \
-    return V8_LIKELY(cmp) ? nullptr                                            \
-                          : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);        \
+    return V8_LIKELY(lhs op rhs) ? 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);                                  \
+      const float lhs, const float rhs, char const* msg);                      \
   extern template V8_BASE_EXPORT std::string*                                  \
-      Check##NAME##Impl<double, double>(double lhs, double rhs,                \
+      Check##NAME##Impl<double, double>(const double lhs, const double rhs,    \
                                         char const* msg);
 DEFINE_CHECK_OP_IMPL(EQ, ==)
 DEFINE_CHECK_OP_IMPL(NE, !=)

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_NOT_NULL(output_handle_);
+    DCHECK(output_handle_ != NULL);
     size_t rv = fwrite(msg, 1, length, output_handle_);
-    DCHECK_EQ(length, rv);
+    DCHECK(static_cast<size_t>(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, 0);
+    DCHECK_GT(arity, 0u);
     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(0, local_type_vec_.size());
+    DCHECK_EQ(0u, 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, stack_.size());
+      DCHECK_LE(stack_depth, static_cast<int>(stack_.size()));
       Value val = Pop();
       stack_.resize(stack_depth);
       return val;

src/wasm/wasm-debug.cc

@@ -47,10 +47,12 @@ FixedArray *GetAsmJsOffsetTables(Handle<WasmDebugInfo> debug_info,
   }
   // Wasm bytes must be valid and must contain asm.js offset table.
   DCHECK(asm_offsets.ok());
-  DCHECK_GE(kMaxInt, asm_offsets.val.size());
+  DCHECK_GE(static_cast<size_t>(kMaxInt), asm_offsets.val.size());
   int num_functions = static_cast<int>(asm_offsets.val.size());
-  DCHECK_EQ(wasm::GetNumberOfFunctions(handle(debug_info->wasm_instance())),
-            num_functions + compiled_module->module()->num_imported_functions);
+  DCHECK_EQ(
+      wasm::GetNumberOfFunctions(handle(debug_info->wasm_instance())),
+      static_cast<int>(num_functions +
+                       compiled_module->module()->num_imported_functions));
   Handle<FixedArray> all_tables =
       isolate->factory()->NewFixedArray(num_functions);
   debug_info->set(kWasmDebugInfoAsmJsOffsets, *all_tables);
@@ -58,7 +60,7 @@ FixedArray *GetAsmJsOffsetTables(Handle<WasmDebugInfo> debug_info,
     std::vector<std::pair<int, int>> &func_asm_offsets = asm_offsets.val[func];
     if (func_asm_offsets.empty()) continue;
     size_t array_size = 2 * kIntSize * func_asm_offsets.size();
-    CHECK_LE(array_size, kMaxInt);
+    CHECK_LE(array_size, static_cast<size_t>(kMaxInt));
     ByteArray *arr =
         *isolate->factory()->NewByteArray(static_cast<int>(array_size));
     all_tables->set(func, arr);

src/wasm/wasm-interpreter.cc

@@ -1173,7 +1173,7 @@ class ThreadImpl : public WasmInterpreter::Thread {
   }
 
   bool DoReturn(InterpreterCode** code, pc_t* pc, pc_t* limit, size_t arity) {
-    DCHECK_GT(frames_.size(), 0);
+    DCHECK_GT(frames_.size(), 0u);
     // Pop all blocks for this frame.
     while (!blocks_.empty() && blocks_.back().fp == frames_.size()) {
       blocks_.pop_back();
@@ -1680,8 +1680,8 @@ class ThreadImpl : public WasmInterpreter::Thread {
   }
 
   WasmVal Pop() {
-    DCHECK_GT(stack_.size(), 0);
-    DCHECK_GT(frames_.size(), 0);
+    DCHECK_GT(stack_.size(), 0u);
+    DCHECK_GT(frames_.size(), 0u);
     DCHECK_GT(stack_.size(), frames_.back().llimit());  // can't pop into locals
     WasmVal val = stack_.back();
     stack_.pop_back();
@@ -1689,8 +1689,8 @@ class ThreadImpl : public WasmInterpreter::Thread {
   }
 
   void PopN(int n) {
-    DCHECK_GE(stack_.size(), n);
-    DCHECK_GT(frames_.size(), 0);
+    DCHECK_GE(stack_.size(), static_cast<size_t>(n));
+    DCHECK_GT(frames_.size(), 0u);
     size_t nsize = stack_.size() - n;
     DCHECK_GE(nsize, frames_.back().llimit());  // can't pop into locals
     stack_.resize(nsize);
@@ -1698,7 +1698,7 @@ class ThreadImpl : public WasmInterpreter::Thread {
 
   WasmVal PopArity(size_t arity) {
     if (arity == 0) return WasmVal();
-    CHECK_EQ(1, arity);
+    CHECK_EQ(1u, 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(module_bytes->length(), offset);
-  DCHECK_GE(module_bytes->length() - offset, size);
+  DCHECK_GE(static_cast<size_t>(module_bytes->length()), offset);
+  DCHECK_GE(static_cast<size_t>(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(0, size % base::OS::CommitPageSize());
+    DCHECK_EQ(0u, size % base::OS::CommitPageSize());
 
     // AllocateGuarded makes the whole region inaccessible by default.
     void* memory = base::OS::AllocateGuarded(alloc_size);
@@ -935,7 +935,7 @@ MaybeHandle<WasmCompiledModule> WasmModule::CompileFunctions(
     // TODO(wasm): only save the sections necessary to deserialize a
     // {WasmModule}. E.g. function bodies could be omitted.
     size_t module_bytes_len = module_end - module_start;
-    DCHECK_LE(module_bytes_len, kMaxInt);
+    DCHECK_LE(module_bytes_len, static_cast<size_t>(kMaxInt));
     Vector<const uint8_t> module_bytes_vec(module_start,
                                            static_cast<int>(module_bytes_len));
     Handle<String> module_bytes_string =
@@ -2063,7 +2063,7 @@ MaybeHandle<WasmModuleObject> wasm::CreateModuleObjectFromBytes(
     compiled_module->set_script(asm_js_script);
     size_t offset_tables_len =
         asm_js_offset_tables_end - asm_js_offset_tables_start;
-    DCHECK_GE(kMaxInt, offset_tables_len);
+    DCHECK_GE(static_cast<size_t>(kMaxInt), offset_tables_len);
     Handle<ByteArray> offset_tables =
         isolate->factory()->NewByteArray(static_cast<int>(offset_tables_len));
     memcpy(offset_tables->GetDataStartAddress(), asm_js_offset_tables_start,

src/wasm/wasm-module.h

@@ -230,7 +230,7 @@ struct V8_EXPORT_PRIVATE WasmModule {
   WasmName GetName(uint32_t offset, uint32_t length) const {
     if (length == 0) return {"<?>", 3};  // no name.
     CHECK(BoundsCheck(offset, offset + length));
-    DCHECK_GE(length, 0);
+    DCHECK_GE(static_cast<int>(length), 0);
     return {reinterpret_cast<const char*>(module_start + offset),
             static_cast<int>(length)};
   }
@@ -244,7 +244,7 @@ struct V8_EXPORT_PRIVATE WasmModule {
   WasmName GetNameOrNull(uint32_t offset, uint32_t length) const {
     if (offset == 0 && length == 0) return {NULL, 0};  // no name.
     CHECK(BoundsCheck(offset, offset + length));
-    DCHECK_GE(length, 0);
+    DCHECK_GE(static_cast<int>(length), 0);
     return {reinterpret_cast<const char*>(module_start + offset),
             static_cast<int>(length)};
   }

src/wasm/wasm-objects.cc

@@ -49,7 +49,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(), kMaxUInt32);
+  CHECK_LE(num->value(), static_cast<double>(kMaxUInt32));
   return static_cast<uint32_t>(num->value());
 }
 
@@ -59,8 +59,8 @@ static int32_t SafeInt32(Object* value) {
   }
   DCHECK(value->IsHeapNumber());
   HeapNumber* num = HeapNumber::cast(value);
-  CHECK_GE(num->value(), Smi::kMinValue);
-  CHECK_LE(num->value(), Smi::kMaxValue);
+  CHECK_GE(num->value(), static_cast<double>(Smi::kMinValue));
+  CHECK_LE(num->value(), static_cast<double>(Smi::kMaxValue));
   return static_cast<int32_t>(num->value());
 }
 
@@ -392,8 +392,9 @@ 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(bytes->length(), function.name_offset);
-  DCHECK_GE(bytes->length() - function.name_offset, function.name_length);
+  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);
   return Vector<const uint8_t>(bytes->GetCharsAddress() + function.name_offset,
                                function.name_length);
 }
@@ -401,7 +402,8 @@ 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(kMaxInt, functions[func_index].code_start_offset);
+  DCHECK_GE(static_cast<uint32_t>(kMaxInt),
+            functions[func_index].code_start_offset);
   return static_cast<int>(functions[func_index].code_start_offset);
 }
 

src/wasm/wasm-text.cc

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