[torque] make overload resolution robust concerning branching contexts

This changes the behavior of overload resolution to not consider if the
call happens in a branching context (i.e., with implicit True and False
labels from a conditional operator or statement).
That way, it is not possible to get different behavior accidentially
by using an operator in the wrong context. Instead, there will be a
compile error because the call happened in a non-branching context, or
because it is ambiguous without this information.

The test doesn't perfectly fit the issue (impossible until we have
negative tests), but instead tests that equality on HeapNumber's works
in boolean contexts, which is something Peter fixed already in
https://crrev.com/c/1432596.


Bug: v8:8737 v8:7793
Change-Id: I08a3801891587aac705dc93b1c65b0c6cf164107
Reviewed-on: https://chromium-review.googlesource.com/c/1456093Reviewed-by: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Daniel Clifford <danno@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59625}

src/builtins/base.tq

@@ -999,6 +999,7 @@ extern macro IntPtrConstant(constexpr int32): intptr;
 extern macro Int32Constant(constexpr int31): int31;
 extern macro Int32Constant(constexpr int32): int32;
 extern macro Float64Constant(constexpr int31): float64;
+extern macro Float64Constant(constexpr float64): float64;
 extern macro SmiConstant(constexpr int31): Smi;
 extern macro SmiConstant(constexpr Smi): Smi;
 extern macro BoolConstant(constexpr bool): bool;
@@ -1074,6 +1075,9 @@ FromConstexpr<uintptr, constexpr int31>(i: constexpr int31): uintptr {
 FromConstexpr<float64, constexpr int31>(i: constexpr int31): float64 {
   return Float64Constant(i);
 }
+FromConstexpr<float64, constexpr float64>(i: constexpr float64): float64 {
+  return Float64Constant(i);
+}
 FromConstexpr<bool, constexpr bool>(b: constexpr bool): bool {
   return BoolConstant(b);
 }

src/torque/implementation-visitor.cc

@@ -1571,16 +1571,9 @@ Callable* ImplementationVisitor::LookupCallable(
     const Signature& signature = overload_signatures[i];
     bool try_bool_context = labels.size() == 0 &&
                             signature.return_type == TypeOracle::GetNeverType();
-    base::Optional<Binding<LocalLabel>*> true_label;
-    base::Optional<Binding<LocalLabel>*> false_label;
-    if (try_bool_context) {
-      true_label = TryLookupLabel(kTrueLabelName);
-      false_label = TryLookupLabel(kFalseLabelName);
-    }
-    if (IsCompatibleSignature(signature, parameter_types, labels) ||
-        (true_label && false_label &&
-         IsCompatibleSignature(signature, parameter_types,
-                               {*true_label, *false_label}))) {
+    if (IsCompatibleSignature(signature, parameter_types, labels.size()) ||
+        (try_bool_context &&
+         IsCompatibleSignature(signature, parameter_types, 2))) {
       candidates.push_back(i);
     }
   }
@@ -1864,7 +1857,7 @@ VisitResult ImplementationVisitor::GeneratePointerCall(
   ParameterTypes types{type->parameter_types(), false};
   Signature sig;
   sig.parameter_types = types;
-  if (!IsCompatibleSignature(sig, parameter_types, {})) {
+  if (!IsCompatibleSignature(sig, parameter_types, 0)) {
     std::stringstream stream;
     stream << "parameters do not match function pointer signature. Expected: ("
            << type->parameter_types() << ") but got: (" << parameter_types
@@ -1914,10 +1907,19 @@ VisitResult ImplementationVisitor::GenerateCall(
   // return but have a True and False label
   if (arguments.labels.size() == 0 &&
       callable->signature().labels.size() == 2) {
-    Binding<LocalLabel>* true_label = LookupLabel(kTrueLabelName);
-    arguments.labels.push_back(true_label);
-    Binding<LocalLabel>* false_label = LookupLabel(kFalseLabelName);
-    arguments.labels.push_back(false_label);
+    base::Optional<Binding<LocalLabel>*> true_label =
+        TryLookupLabel(kTrueLabelName);
+    base::Optional<Binding<LocalLabel>*> false_label =
+        TryLookupLabel(kFalseLabelName);
+    if (!true_label || !false_label) {
+      ReportError(
+          callable->ReadableName(),
+          " does not return a value, but has to be called in a branching "
+          "context (e.g., conditional or if-condition). You can fix this by "
+          "adding \"? true : false\".");
+    }
+    arguments.labels.push_back(*true_label);
+    arguments.labels.push_back(*false_label);
   }
 
   const Type* return_type = callable->signature().return_type;
@@ -2408,15 +2410,11 @@ DEFINE_CONTEXTUAL_VARIABLE(ImplementationVisitor::CurrentReturnValue)
 DEFINE_CONTEXTUAL_VARIABLE(ImplementationVisitor::CurrentConstructorInfo)
 
 bool IsCompatibleSignature(const Signature& sig, const TypeVector& types,
-                           const std::vector<Binding<LocalLabel>*>& labels) {
+                           size_t label_count) {
   auto i = sig.parameter_types.types.begin() + sig.implicit_count;
   if ((sig.parameter_types.types.size() - sig.implicit_count) > types.size())
     return false;
-  // TODO(danno): The test below is actually insufficient. The labels'
-  // parameters must be checked too. ideally, the named part of
-  // LabelDeclarationVector would be factored out so that the label count and
-  // parameter types could be passed separately.
-  if (sig.labels.size() != labels.size()) return false;
+  if (sig.labels.size() != label_count) return false;
   for (auto current : types) {
     if (i == sig.parameter_types.types.end()) {
       if (!sig.parameter_types.var_args) return false;

src/torque/implementation-visitor.h

@@ -201,8 +201,9 @@ struct Arguments {
   std::vector<Binding<LocalLabel>*> labels;
 };
 
+// Determine if a callable should be considered as an overload.
 bool IsCompatibleSignature(const Signature& sig, const TypeVector& types,
-                           const std::vector<Binding<LocalLabel>*>& labels);
+                           size_t label_count);
 
 class ImplementationVisitor : public FileVisitor {
  public:

test/cctest/torque/test-torque.cc

@@ -275,6 +275,22 @@ TEST(TestGenericOverload) {
   ft.Call();
 }
 
+TEST(TestEquality) {
+  CcTest::InitializeVM();
+  Isolate* isolate(CcTest::i_isolate());
+  i::HandleScope scope(isolate);
+  Handle<Context> context =
+      Utils::OpenHandle(*v8::Isolate::GetCurrent()->GetCurrentContext());
+  CodeAssemblerTester asm_tester(isolate, 0);
+  TestTorqueAssembler m(asm_tester.state());
+  {
+    m.TestEquality(m.UncheckedCast<Context>(m.HeapConstant(context)));
+    m.Return(m.UndefinedConstant());
+  }
+  FunctionTester ft(asm_tester.GenerateCode(), 0);
+  ft.Call();
+}
+
 TEST(TestLogicalOperators) {
   Isolate* isolate(CcTest::InitIsolateOnce());
   CodeAssemblerTester asm_tester(isolate, 0);

test/torque/test-torque.tq

@@ -498,6 +498,15 @@ namespace test {
     check(UnsafeCast<Smi>(ExampleGenericOverload<Object>(xObject)) == 5);
   }
 
+  macro TestEquality(implicit context: Context)() {
+    const notEqual: bool =
+        AllocateHeapNumberWithValue(0.5) != AllocateHeapNumberWithValue(0.5);
+    check(!notEqual);
+    const equal: bool =
+        AllocateHeapNumberWithValue(0.5) == AllocateHeapNumberWithValue(0.5);
+    check(equal);
+  }
+
   macro BoolToBranch(x: bool): never
       labels Taken, NotTaken {
     if (x) {