[torque] disallow using logical operators in value contexts

This CL makes sure, that logical operators (||, &&) always have return
type never. Together with a check that never is never passed as a
function argument, this prevents faulty evaluation as in !(x || y).

Before, the logical operators had a behavior similar to
(bool labels Taken, NotTaken), with a fast exit if the left-hand side
allowed shor-circuit evaluation, but returning the right-hand side
otherwise. Since we want to allow existing (a || b || c) patterns in
the codebase, this requires weakening the restriction that the left-
and right-hand side need to have the same type. Now the possibilites
are:
bool, never
never, bool
never, never
bool, bool
constexpr bool, constexpr bool

Bug: v8:8137
Change-Id: I9576b337dc4008ac58b4625e77fef4e73bcdd6e3
Reviewed-on: https://chromium-review.googlesource.com/1215162Reviewed-by: Daniel Clifford <danno@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55750}

src/torque/implementation-visitor.cc

@@ -411,25 +411,40 @@ VisitResult ImplementationVisitor::Visit(LogicalOrExpression* expr) {
       GenerateIndent();
       source_out() << "GotoIf(" << RValueFlattenStructs(left_result) << ", "
                    << true_label->generated() << ");\n";
-    } else if (!left_result.type()->IsConstexprBool()) {
+    } else if (left_result.type()->IsNever()) {
       GenerateLabelBind(false_label);
+    } else if (!left_result.type()->IsConstexprBool()) {
+      ReportError(
+          "expected type bool, constexpr bool, or never on left-hand side of "
+          "operator ||");
     }
   }
-  VisitResult right_result = Visit(expr->right);
-  if (right_result.type() != left_result.type()) {
-    std::stringstream stream;
-    stream << "types of left and right expression of logical OR don't match (\""
-           << *left_result.type() << "\" vs. \"" << *right_result.type()
-           << "\")";
-    ReportError(stream.str());
-  }
+
   if (left_result.type()->IsConstexprBool()) {
+    VisitResult right_result = Visit(expr->right);
+    if (!right_result.type()->IsConstexprBool()) {
+      ReportError(
+          "expected type constexpr bool on right-hand side of operator "
+          "||");
+    }
     return VisitResult(left_result.type(),
                        std::string("(") + RValueFlattenStructs(left_result) +
                            " || " + RValueFlattenStructs(right_result) + ")");
-  } else {
-    return right_result;
   }
+
+  VisitResult right_result = Visit(expr->right);
+  if (right_result.type()->IsBool()) {
+    Label* true_label = declarations()->LookupLabel(kTrueLabelName);
+    Label* false_label = declarations()->LookupLabel(kFalseLabelName);
+    source_out() << "Branch(" << RValueFlattenStructs(right_result) << ", "
+                 << true_label->generated() << ", " << false_label->generated()
+                 << ");\n";
+    return VisitResult(TypeOracle::GetNeverType(), "/*never*/");
+  } else if (!right_result.type()->IsNever()) {
+    ReportError(
+        "expected type bool or never on right-hand side of operator ||");
+  }
+  return right_result;
 }
 
 VisitResult ImplementationVisitor::Visit(LogicalAndExpression* expr) {
@@ -444,25 +459,40 @@ VisitResult ImplementationVisitor::Visit(LogicalAndExpression* expr) {
       GenerateIndent();
       source_out() << "GotoIfNot(" << RValueFlattenStructs(left_result) << ", "
                    << false_label->generated() << ");\n";
-    } else if (!left_result.type()->IsConstexprBool()) {
+    } else if (left_result.type()->IsNever()) {
       GenerateLabelBind(true_label);
+    } else if (!left_result.type()->IsConstexprBool()) {
+      ReportError(
+          "expected type bool, constexpr bool, or never on left-hand side of "
+          "operator &&");
     }
   }
-  VisitResult right_result = Visit(expr->right);
-  if (right_result.type() != left_result.type()) {
-    std::stringstream stream;
-    stream
-        << "types of left and right expression of logical AND don't match (\""
-        << *left_result.type() << "\" vs. \"" << *right_result.type() << "\")";
-    ReportError(stream.str());
-  }
+
   if (left_result.type()->IsConstexprBool()) {
+    VisitResult right_result = Visit(expr->right);
+    if (!right_result.type()->IsConstexprBool()) {
+      ReportError(
+          "expected type constexpr bool on right-hand side of operator "
+          "&&");
+    }
     return VisitResult(left_result.type(),
                        std::string("(") + RValueFlattenStructs(left_result) +
                            " && " + RValueFlattenStructs(right_result) + ")");
-  } else {
-    return right_result;
   }
+
+  VisitResult right_result = Visit(expr->right);
+  if (right_result.type()->IsBool()) {
+    Label* true_label = declarations()->LookupLabel(kTrueLabelName);
+    Label* false_label = declarations()->LookupLabel(kFalseLabelName);
+    source_out() << "Branch(" << RValueFlattenStructs(right_result) << ", "
+                 << true_label->generated() << ", " << false_label->generated()
+                 << ");\n";
+    return VisitResult(TypeOracle::GetNeverType(), "/*never*/");
+  } else if (!right_result.type()->IsNever()) {
+    ReportError(
+        "expected type bool or never on right-hand side of operator &&");
+  }
+  return right_result;
 }
 
 VisitResult ImplementationVisitor::Visit(IncrementDecrementExpression* expr) {
@@ -1859,16 +1889,16 @@ VisitResult ImplementationVisitor::Visit(CallExpression* expr,
                                          bool is_tailcall) {
   Arguments arguments;
   std::string name = expr->callee.name;
-    TypeVector specialization_types =
-        GetTypeVector(expr->callee.generic_arguments);
-    bool has_template_arguments = !specialization_types.empty();
-    for (Expression* arg : expr->arguments)
-      arguments.parameters.push_back(Visit(arg));
-    arguments.labels = LabelsFromIdentifiers(expr->labels);
-    VisitResult result;
-    if (!has_template_arguments &&
-        declarations()->Lookup(expr->callee.name)->IsValue()) {
-      result = GeneratePointerCall(&expr->callee, arguments, is_tailcall);
+  TypeVector specialization_types =
+      GetTypeVector(expr->callee.generic_arguments);
+  bool has_template_arguments = !specialization_types.empty();
+  for (Expression* arg : expr->arguments)
+    arguments.parameters.push_back(Visit(arg));
+  arguments.labels = LabelsFromIdentifiers(expr->labels);
+  VisitResult result;
+  if (!has_template_arguments &&
+      declarations()->Lookup(expr->callee.name)->IsValue()) {
+    result = GeneratePointerCall(&expr->callee, arguments, is_tailcall);
   } else {
     result = GenerateCall(name, arguments, specialization_types, is_tailcall);
   }
@@ -1935,6 +1965,10 @@ bool ImplementationVisitor::GenerateExpressionBranch(
 
 VisitResult ImplementationVisitor::GenerateImplicitConvert(
     const Type* destination_type, VisitResult source) {
+  if (source.type() == TypeOracle::GetNeverType()) {
+    ReportError("it is not allowed to use a value of type never");
+  }
+
   if (destination_type == source.type()) {
     return source;
   }

test/cctest/torque/test-torque.cc

@@ -259,6 +259,18 @@ TEST(TestGenericOverload) {
   ft.Call();
 }
 
+TEST(TestLogicalOperators) {
+  Isolate* isolate(CcTest::InitIsolateOnce());
+  CodeAssemblerTester asm_tester(isolate, 0);
+  TestBuiltinsFromDSLAssembler m(asm_tester.state());
+  {
+    m.TestLogicalOperators();
+    m.Return(m.UndefinedConstant());
+  }
+  FunctionTester ft(asm_tester.GenerateCode(), 0);
+  ft.Call();
+}
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8

test/torque/test-torque.tq

@@ -446,4 +446,88 @@ module test {
     check(ExampleGenericOverload<Smi>(x_smi) == 6);
     check(unsafe_cast<Smi>(ExampleGenericOverload<Object>(x_object)) == 5);
   }
+
+  macro BoolToBranch(x : bool) : never labels Taken, NotTaken {
+    if (x) {
+      goto Taken;
+    } else {
+      goto NotTaken;
+    }
+  }
+
+  macro TestOrAnd1(x : bool, y : bool, z : bool) : bool {
+    return BoolToBranch(x) || y && z ? true : false;
+  }
+
+  macro TestOrAnd2(x : bool, y : bool, z : bool) : bool {
+    return x || BoolToBranch(y) && z ? true : false;
+  }
+
+  macro TestOrAnd3(x : bool, y : bool, z : bool) : bool {
+    return x || y && BoolToBranch(z) ? true : false;
+  }
+
+  macro TestAndOr1(x : bool, y : bool, z : bool) : bool {
+    return BoolToBranch(x) && y || z ? true : false;
+  }
+
+  macro TestAndOr2(x : bool, y : bool, z : bool) : bool {
+    return x && BoolToBranch(y) || z ? true : false;
+  }
+
+  macro TestAndOr3(x : bool, y : bool, z : bool) : bool {
+    return x && y || BoolToBranch(z) ? true : false;
+  }
+
+  macro TestLogicalOperators() {
+    check(TestAndOr1(true, true, true));
+    check(TestAndOr2(true, true, true));
+    check(TestAndOr3(true, true, true));
+    check(TestAndOr1(true, true, false));
+    check(TestAndOr2(true, true, false));
+    check(TestAndOr3(true, true, false));
+    check(TestAndOr1(true, false, true));
+    check(TestAndOr2(true, false, true));
+    check(TestAndOr3(true, false, true));
+    check(!TestAndOr1(true, false, false));
+    check(!TestAndOr2(true, false, false));
+    check(!TestAndOr3(true, false, false));
+    check(TestAndOr1(false, true, true));
+    check(TestAndOr2(false, true, true));
+    check(TestAndOr3(false, true, true));
+    check(!TestAndOr1(false, true, false));
+    check(!TestAndOr2(false, true, false));
+    check(!TestAndOr3(false, true, false));
+    check(TestAndOr1(false, false, true));
+    check(TestAndOr2(false, false, true));
+    check(TestAndOr3(false, false, true));
+    check(!TestAndOr1(false, false, false));
+    check(!TestAndOr2(false, false, false));
+    check(!TestAndOr3(false, false, false));
+    check(TestOrAnd1(true, true, true));
+    check(TestOrAnd2(true, true, true));
+    check(TestOrAnd3(true, true, true));
+    check(TestOrAnd1(true, true, false));
+    check(TestOrAnd2(true, true, false));
+    check(TestOrAnd3(true, true, false));
+    check(TestOrAnd1(true, false, true));
+    check(TestOrAnd2(true, false, true));
+    check(TestOrAnd3(true, false, true));
+    check(TestOrAnd1(true, false, false));
+    check(TestOrAnd2(true, false, false));
+    check(TestOrAnd3(true, false, false));
+    check(TestOrAnd1(false, true, true));
+    check(TestOrAnd2(false, true, true));
+    check(TestOrAnd3(false, true, true));
+    check(!TestOrAnd1(false, true, false));
+    check(!TestOrAnd2(false, true, false));
+    check(!TestOrAnd3(false, true, false));
+    check(!TestOrAnd1(false, false, true));
+    check(!TestOrAnd2(false, false, true));
+    check(!TestOrAnd3(false, false, true));
+    check(!TestOrAnd1(false, false, false));
+    check(!TestOrAnd2(false, false, false));
+    check(!TestOrAnd3(false, false, false));
+  }
+
 }