Fix semantics bugs

src/libdredd/include/libdredd/mutation_replace_expr.h

@@ -167,7 +167,6 @@ class MutationReplaceExpr : public Mutation {
 
   void ReplaceExprWithFunctionCall(const std::string& new_function_name,
                                    const std::string& input_type,
-                                   bool semantics_preserving_mutation,
                                    int local_mutation_id,
                                    clang::ASTContext& ast_context,
                                    const clang::Preprocessor& preprocessor,

src/libdredd/src/mutate_ast_consumer.cc

@@ -150,7 +150,7 @@ void MutateAstConsumer::HandleTranslationUnit(clang::ASTContext& ast_context) {
   assert(!rewriter_result && "Rewrite failed.\n");
 
   if (semantics_preserving_mutation_) {
-    // TODO(JamesLeeJones): Maybe move later.
+    // TODO(JLJ): This should probably be moved to the prelude function.
     rewriter_result = rewriter_.InsertTextBefore(
         start_location_of_first_function_in_source_file,
         "static unsigned long long int no_op = 0;\n\n");

src/libdredd/src/mutate_visitor.cc

@@ -406,7 +406,6 @@ void MutateVisitor::HandleExpr(clang::Expr* expr) {
     return;
   }
 
-
   // It is incorrect to attempt to mutate braced initializer lists.
   if (llvm::dyn_cast<clang::InitListExpr>(expr) != nullptr) {
     return;

src/libdredd/src/mutation_replace_binary_operator.cc

@@ -523,37 +523,23 @@ MutationReplaceBinaryOperator::GenerateBinaryOperatorReplacementMacro(
         safe_math_check += " && ";
       }
       break;
-    case clang::BO_LAnd:
-      // It is only safe to replace || with && if the first argument is false
-      // otherwise the second argument wouldn't normally be evaluated and doing
-      // so may be dangerous.
-      safe_math_check += "!(arg1) &&";
-      break;
-    case clang::BO_LOr:
-      // It is only safe to replace && with || if the first argument is true
-      // otherwise the second argument wouldn't normally be evaluated and doing
-      // so may be dangerous.
-      safe_math_check += "(arg1) &&";
-      break;
-    case clang::BO_Cmp:
-    case clang::BO_NE:
-      if (binary_operator_->getOpcode() == clang::BinaryOperatorKind::BO_LAnd) {
-        // It is only safe to replace && with a comparison operator if the first
-        // argument is true otherwise the second argument wouldn't normally be
-        // evaluated and doing so may be dangerous.
-        safe_math_check += "(arg1) && ";
-      } else if (binary_operator_->getOpcode() ==
-                 clang::BinaryOperatorKind::BO_LOr) {
-        // It is only safe to replace || with a comparison operator if the first
-        // argument is false otherwise the second argument wouldn't normally be
-        // evaluated and doing so may be dangerous.
-        safe_math_check += "!(arg1) && ";
-      }
-      break;
     default:
       break;
   }
 
+  if (binary_operator_->getOpcode() == clang::BinaryOperatorKind::BO_LAnd) {
+    // It is only safe to replace && with a comparison operator if the first
+    // argument is true otherwise the second argument wouldn't normally be
+    // evaluated and doing so may be dangerous.
+    safe_math_check += "(arg1) && ";
+  } else if (binary_operator_->getOpcode() ==
+             clang::BinaryOperatorKind::BO_LOr) {
+    // It is only safe to replace || with a comparison operator if the first
+    // argument is false otherwise the second argument wouldn't normally be
+    // evaluated and doing so may be dangerous.
+    safe_math_check += "!(arg1) && ";
+  }
+
   const std::string result = "#define " + name + "(arg1, arg2) if (" +
                              safe_math_check + "(" +
                              ConvertToSemanticsPreservingBinaryExpression(
@@ -847,31 +833,55 @@ std::string MutationReplaceBinaryOperator::GenerateMutatorFunction(
   }
 
   if (!only_track_mutant_coverage) {
-    // If the first operand to a logical operator is side-effecting, store the
-    // result to avoid having to evaluate it multiple times.
-    if (semantics_preserving_mutation && binary_operator_->isLogicalOp() &&
-        binary_operator_->getLHS()->HasSideEffects(ast_context)) {
-      new_function << "  " + lhs_type + " arg1_evaluated = " + arg1_evaluated +
-                          ";\n";
-      arg1_evaluated = "arg1_evaluated";
-    }
+    if (semantics_preserving_mutation) {
+      // For assignment operators, we must store the value of arg1 before
+      // assigning to it so that the mutant killing checks are correct.
+      // TODO(JLJ): For now we will not store the result of evaluating the right
+      // argument to avoid evaluating it multiple time but check if this is
+      // actually necessary (probably not).
+      if (binary_operator_->isAssignmentOp()) {
+        // We need to assign to a non reference type to avoid the copy getting
+        // updated.
+        // TODO(JLJ): The knowledge that the last char of the type of an
+        // assignment will always be & or * is duplicated.
+        std::string non_reference_type =
+            lhs_type.substr(0, lhs_type.size() - 1);
+        new_function << "  " << non_reference_type
+                     << " arg1_original = " << arg1_evaluated << ";\n";
+      }
 
-    // If the second operand to a logical operator is side-effecting, store the
-    // result only if the second operand would normally be evaluated to avoid
-    // having to evaluate it multiple times.
-    if (semantics_preserving_mutation && binary_operator_->isLogicalOp() &&
-        binary_operator_->getRHS()->HasSideEffects(ast_context)) {
-      new_function << "  " + rhs_type + " arg2_evaluated;\n";
-      new_function << "  if (";
-      // TODO(JLJ): This is hidden in too many places.
-      if (binary_operator_->getOpcode() == clang::BinaryOperatorKind::BO_LAnd) {
-        new_function << arg1_evaluated;
-      } else if (binary_operator_->getOpcode() ==
-                 clang::BinaryOperatorKind::BO_LOr) {
-        new_function << "!" + arg1_evaluated;
+      // If the first operand to a logical operator is side-effecting, store the
+      // result to avoid having to evaluate it multiple times.
+      if (binary_operator_->getLHS()->HasSideEffects(ast_context)) {
+        new_function << "  " << lhs_type
+                     << " arg1_evaluated = " << arg1_evaluated << ";\n";
+        arg1_evaluated = "arg1_evaluated";
+      }
+
+      // If the second operand to a logical operator is side-effecting, store
+      // the result only if the second operand would normally be evaluated to
+      // avoid having to evaluate it multiple times.
+      if (binary_operator_->getRHS()->HasSideEffects(ast_context)) {
+        new_function << "  " << rhs_type << " arg2_evaluated";
+
+        // For logical operators, we can't guarantee the second operand will be
+        // evaluated, so we have to treat them differently.
+        if (binary_operator_->isLogicalOp()) {
+          new_function << ";\n  if (";
+          // TODO(JLJ): This is hidden in too many places.
+          if (binary_operator_->getOpcode() ==
+              clang::BinaryOperatorKind::BO_LAnd) {
+            new_function << arg1_evaluated;
+          } else if (binary_operator_->getOpcode() ==
+                     clang::BinaryOperatorKind::BO_LOr) {
+            new_function << "!" << arg1_evaluated;
+          }
+          new_function << ") arg2_evaluated = " << arg2_evaluated << ";\n";
+        } else {
+          new_function << " = " << arg2_evaluated << ";\n";
+        }
+        arg2_evaluated = "arg2_evaluated";
       }
-      new_function << ") arg2_evaluated = " + arg2_evaluated + ";\n";
-      arg2_evaluated = "arg2_evaluated";
     }
 
     // Quickly apply the original operator if no mutant is enabled (which will
@@ -883,6 +893,12 @@ std::string MutationReplaceBinaryOperator::GenerateMutatorFunction(
                  << " " << arg2_evaluated;
     if (semantics_preserving_mutation) {
       new_function << "," << result_type;
+      if (binary_operator_->isAssignmentOp()) {
+        // After doing the original mutation with MUTATION_PRELUDE, use
+        // arg1_original so the mutants are checked against the value of arg1
+        // before being assigned to.
+        arg1_evaluated = "arg1_original";
+      }
     }
 
     new_function << ");\n";

src/libdredd/src/mutation_replace_expr.cc

@@ -528,7 +528,7 @@ std::string MutationReplaceExpr::GenerateMutatorFunction(
     int& mutation_id, protobufs::MutationReplaceExpr& protobuf_message) const {
   std::stringstream new_function;
   new_function << "static " << result_type << " " << function_name << "(";
-  if (!semantics_preserving_mutation && ast_context.getLangOpts().CPlusPlus &&
+  if (ast_context.getLangOpts().CPlusPlus &&
       expr_->HasSideEffects(ast_context)) {
     new_function << "std::function<" << input_type << "()>";
   } else {
@@ -537,7 +537,7 @@ std::string MutationReplaceExpr::GenerateMutatorFunction(
   new_function << " arg, int local_mutation_id) {\n";
 
   std::string arg_evaluated = "arg";
-  if (!semantics_preserving_mutation && ast_context.getLangOpts().CPlusPlus &&
+  if (ast_context.getLangOpts().CPlusPlus &&
       expr_->HasSideEffects(ast_context)) {
     arg_evaluated += "()";
   }
@@ -547,6 +547,14 @@ std::string MutationReplaceExpr::GenerateMutatorFunction(
   }
 
   if (!only_track_mutant_coverage) {
+    // If the expression has side-effects, store the result of evaluating it so
+    // we don't evaluate it multiple times.
+    if (expr_->HasSideEffects(ast_context)) {
+      new_function << "  " << input_type << " arg_evaluated = " << arg_evaluated
+                   << ";\n";
+      arg_evaluated = "arg_evaluated";
+    }
+
     // Quickly apply the original operator if no mutant is enabled (which will
     // be the common case).
     new_function << "  MUTATION_PRELUDE(" << arg_evaluated;
@@ -611,9 +619,8 @@ void MutationReplaceExpr::ApplyCTypeModifiers(const clang::Expr& expr,
 
 void MutationReplaceExpr::ReplaceExprWithFunctionCall(
     const std::string& new_function_name, const std::string& input_type,
-    bool semantics_preserving_mutation, int local_mutation_id,
-    clang::ASTContext& ast_context, const clang::Preprocessor& preprocessor,
-    clang::Rewriter& rewriter) const {
+    int local_mutation_id, clang::ASTContext& ast_context,
+    const clang::Preprocessor& preprocessor, clang::Rewriter& rewriter) const {
   // Replacement of an expression with a function call is simulated by
   // Inserting suitable text before and after the expression.
   // This is preferable over the (otherwise more intuitive) approach of directly
@@ -627,31 +634,26 @@ void MutationReplaceExpr::ReplaceExprWithFunctionCall(
 
   if (ast_context.getLangOpts().CPlusPlus &&
       expr_->HasSideEffects(ast_context)) {
-    if (!semantics_preserving_mutation) {
-      prefix.append(+"[&]() -> " + input_type + " { return ");
-    }
+    prefix.append(+"[&]() -> " + input_type + " { return ");
     prefix.append(  // We don't need to static cast constant expressions
         (IsCxx11ConstantExpr(*expr_, ast_context)
              ? ""
              : "static_cast<" + input_type + ">("));
     suffix.append(IsCxx11ConstantExpr(*expr_, ast_context) ? "" : ")");
-    if (!semantics_preserving_mutation) {
-      suffix.append("; }");
-    }
+    suffix.append("; }");
   }
 
-  if (!ast_context.getLangOpts().CPlusPlus) {
-    if (expr_->isLValue() && input_type.ends_with('*')) {
-      prefix.append("&(");
-      suffix.append(")");
-    } else if (const auto* binary_expr =
-                   llvm::dyn_cast<clang::BinaryOperator>(expr_)) {
-      // The comma operator requires special care in C, to avoid it appearing to
-      // provide multiple parameters for an enclosing function call.
-      if (binary_expr->isCommaOp()) {
-        prefix.append("(");
-        suffix.append(")");
-      }
+  if (!ast_context.getLangOpts().CPlusPlus && expr_->isLValue() &&
+      input_type.ends_with('*')) {
+    prefix.append("&(");
+    suffix = ")" + suffix;
+  }
+  if (const auto* binary_expr = llvm::dyn_cast<clang::BinaryOperator>(expr_)) {
+    // The comma operator requires special care in C, to avoid it appearing to
+    // provide multiple parameters for an enclosing function call.
+    if (binary_expr->isCommaOp()) {
+      prefix.append("(");
+      suffix = ")" + suffix;
     }
   }
 
@@ -734,7 +736,6 @@ protobufs::MutationGroup MutationReplaceExpr::Apply(
   // Subtracting |first_mutation_id_in_file| turns the global mutation id,
   // |mutation_id|, into a file-local mutation id.
   ReplaceExprWithFunctionCall(new_function_name, input_type,
-                              semantics_preserving_mutation,
                               mutation_id - first_mutation_id_in_file,
                               ast_context, preprocessor, rewriter);
 

src/libdredd/src/mutation_replace_unary_operator.cc

@@ -192,6 +192,7 @@ std::string MutationReplaceUnaryOperator::GenerateMutatorFunction(
   }
 
   new_function << "static " << result_type << " " << function_name << "(";
+  // We don't need to use lambdas if the mutations are semantics preserving because the unary operator will always be evaluated anyway.
   if (!semantics_preserving_mutation && ast_context.getLangOpts().CPlusPlus &&
       unary_operator_->HasSideEffects(ast_context)) {
     new_function << "std::function<" << input_type << "()>";
@@ -201,6 +202,7 @@ std::string MutationReplaceUnaryOperator::GenerateMutatorFunction(
   new_function << " arg, int local_mutation_id) {\n";
 
   std::string arg_evaluated = "arg";
+  // We don't need to use lambdas if the mutations are semantics preserving because the unary operator will always be evaluated anyway.
   if (!semantics_preserving_mutation && ast_context.getLangOpts().CPlusPlus &&
       unary_operator_->HasSideEffects(ast_context)) {
     arg_evaluated += "()";
@@ -217,8 +219,14 @@ std::string MutationReplaceUnaryOperator::GenerateMutatorFunction(
     if (semantics_preserving_mutation &&
         (unary_operator_->getOpcode() == clang::UnaryOperatorKind::UO_PreInc ||
          unary_operator_->getOpcode() == clang::UnaryOperatorKind::UO_PreDec)) {
-      new_function << "  " << result_type << " arg_original = " << arg_evaluated
-                   << ";\n";
+      // We need to assign to a non reference type to avoid the copy getting
+      // updated.
+      // TODO(JLJ): The knowledge that the last char of the type of an
+      // assignment will always be & or * is duplicated.
+      std::string non_reference_type =
+          result_type.substr(0, result_type.size() - 1);
+      new_function << "  " << non_reference_type
+                   << " arg_original = " << arg_evaluated << ";\n";
     }
 
     new_function << "  MUTATION_PRELUDE(";
@@ -241,6 +249,8 @@ std::string MutationReplaceUnaryOperator::GenerateMutatorFunction(
     new_function << ");\n";
   }
 
+  // TODO(JLJ): Come up with a better way than repeating this either side of
+  // MUTATION_PRELUDE. P.S I think this is done elsewhere.
   if (semantics_preserving_mutation &&
       (unary_operator_->getOpcode() == clang::UnaryOperatorKind::UO_PreInc ||
        unary_operator_->getOpcode() == clang::UnaryOperatorKind::UO_PreDec)) {
@@ -514,6 +524,7 @@ protobufs::MutationGroup MutationReplaceUnaryOperator::Apply(
   // These record the text that should be inserted before and after the operand.
   std::string prefix = new_function_name + "(";
   std::string suffix;
+  // We don't need to use lambdas if the mutations are semantics preserving because the unary operator will always be evaluated anyway.
   if (!semantics_preserving_mutation && ast_context.getLangOpts().CPlusPlus &&
       unary_operator_->HasSideEffects(ast_context)) {
     prefix.append(

src/libdredd/src/util.cc

@@ -140,7 +140,7 @@ std::string GenerateMutationPrelude(bool semantics_preserving_mutation) {
            "return arg\n";
   }
 
-  return result + ",type) type actual_result = arg;\n";
+  return result + ",type) type actual_result = (arg);\n";
 }
 
 std::string GenerateUnaryMacroCall(const std::string& macro_name,