Refine logic for avoiding mutations in presence of temporaries

Fixes #330.

.github/workflows/cxx_apps.sh

@@ -157,7 +157,7 @@ pushd SPIRV-Tools
   ./build/test/val/test_val_fghijklmnop
   ./build/test/val/test_val_rstuvw
   NUM_MUTANTS=`python3 ${DREDD_ROOT}/scripts/query_mutant_info.py mutation-info.json --largest-mutant-id`
-  EXPECTED_NUM_MUTANTS=68839
+  EXPECTED_NUM_MUTANTS=68467
   if [ ${NUM_MUTANTS} -ne ${EXPECTED_NUM_MUTANTS} ]
   then
      echo "Found ${NUM_MUTANTS} mutants when mutating the SPIR-V validator source code. Expected ${EXPECTED_NUM_MUTANTS}. If Dredd changed recently, the expected value may just need to be updated, if it still looks sensible. Otherwise, there is likely a problem."
@@ -183,7 +183,7 @@ pushd llvm-project
   ${DREDD_EXECUTABLE} --mutation-info-file mutation-info.json -p "${DREDD_ROOT}/llvm-project/build" "${FILES[@]}"
   cmake --build build --target LLVMInstCombine
   NUM_MUTANTS=`python3 ${DREDD_ROOT}/scripts/query_mutant_info.py mutation-info.json --largest-mutant-id`
-  EXPECTED_NUM_MUTANTS=98042
+  EXPECTED_NUM_MUTANTS=98033
   if [ ${NUM_MUTANTS} -ne ${EXPECTED_NUM_MUTANTS} ]
   then
      echo "Found ${NUM_MUTANTS} mutants when mutating the LLVM source code. Expected ${EXPECTED_NUM_MUTANTS}. If Dredd changed recently, the expected value may just need to be updated, if it still looks sensible. Otherwise, there is likely a problem."

scripts/check_one_execute_test.py

@@ -54,6 +54,9 @@
 
 # Mutate the program using Dredd.
 cmd = [DREDD_INSTALLED_EXECUTABLE, '--mutation-info-file', 'temp.json', f'tomutate.{extension}', '--']
+if test_is_cxx:
+    # This supports execute tests that use C++ 20 features.
+    cmd.append('--std=c++20')
 dredd_result = subprocess.run(cmd)
 if dredd_result.returncode != 0:
     print("Dredd failed.")

src/libdredd/include_private/include/libdredd/mutate_visitor.h

@@ -229,23 +229,25 @@ class MutateVisitor : public clang::RecursiveASTVisitor<MutateVisitor> {
   // results of) argument-dependent lookup.
   bool MutatingMayAffectArgumentDependentLookup(const clang::Expr& expr) const;
 
-  // A subtle issue can arise when a call expression takes materialized
-  // temporaries as an argument and returns an l-value. It might be that the
-  // returned l-value is a reference to temporary storage. This happens for
-  // instance with an expression like:
+  // A subtle issue can arise when an expression has subexpressions that
+  // correspond to temporary objects. For instance, consider an expression such
+  // as:
   //
   //    foo()[0]
   //
   // where function foo returns a std::vector. The array operator yields an
   // element reference, which is a reference into the temporary object
   // associated with the vector returned by foo.
   //
-  // We must avoid mutating such calls, because this will end up with a lambda
-  // to provide the call result, and that lambda will end up yielding a
-  // reference to a temporary, which is invalid.
+  // We must avoid mutating such expressions, because doing so will lead to
+  // the original expression being provided to a mutator function via lambda.
+  // Because the temporary object will no longer be in the same scope as the
+  // reference that ultimately depends on it (in this case the lvalue to rvalue
+  // cast that turns the array reference into a value), the temporary object
+  // will be destroyed too early.
   //
-  // This function checks for this case.
-  static bool IsLvalueCallThatUsesMaterializedTemporary(
+  // This function checks for such cases.
+  static bool MayDependOnLifetimeOfMaterializedTemporaryStorage(
       const clang::Expr& expr);
 
   const clang::CompilerInstance* compiler_instance_;

src/libdredd/src/mutate_visitor.cc

@@ -653,7 +653,7 @@ bool MutateVisitor::VisitExpr(clang::Expr* expr) {
     return true;
   }
 
-  if (IsLvalueCallThatUsesMaterializedTemporary(*expr)) {
+  if (MayDependOnLifetimeOfMaterializedTemporaryStorage(*expr)) {
     // Do not mutate a call expression if there is a risk that the value
     // returned by the call might be a reference to temporary storage arising
     // from a materialized temporary expression.
@@ -837,25 +837,42 @@ bool MutateVisitor::MutatingMayAffectArgumentDependentLookup(
   return false;
 }
 
-bool MutateVisitor::IsLvalueCallThatUsesMaterializedTemporary(
+bool MutateVisitor::MayDependOnLifetimeOfMaterializedTemporaryStorage(
     const clang::Expr& expr) {
-  if (!expr.isLValue()) {
-    // Not an l-value: not a problem.
-    return false;
+  if (llvm::dyn_cast<clang::MaterializeTemporaryExpr>(&expr) != nullptr) {
+    // Base case: this is a materialized temporary.
+    return true;
   }
-  const auto* call_expr = llvm::dyn_cast<clang::CallExpr>(&expr);
-  if (call_expr == nullptr) {
-    // Not a call: not a problem.
+
+  if (const auto* member_expr = llvm::dyn_cast<clang::MemberExpr>(&expr)) {
+    // Consider a member expression whose base expression may depend on the
+    // lifetime of a materialized temporary. If the member expression yields
+    // an lvalue or a pointer, then the member expression itself may depend on
+    // the lifetime of the materialized temporary.
+    if (member_expr->isLValue() || member_expr->getType()->isPointerType()) {
+      return MayDependOnLifetimeOfMaterializedTemporaryStorage(
+          *member_expr->getBase());
+    }
     return false;
   }
-  // We have a call that yields an l-value. Confirm that none of the call
-  // arguments are materialized temporaries.
-  for (const auto* arg : call_expr->arguments()) {
-    if (llvm::dyn_cast<clang::MaterializeTemporaryExpr>(arg) != nullptr) {
-      // This is the case that we need to avoid.
-      return true;
+
+  if (const auto* call_expr = llvm::dyn_cast<clang::CallExpr>(&expr)) {
+    // Consider a calll expression that returns an lvalue or pointer. If some
+    // argument to the call depends on the lifetime of a materialized temporary
+    // then the overall call expression may do so too. In practice this occurs
+    // in the context of the [] operator on a std::vector. When the target
+    // vector is a temporary, the [] operator may yield a reference into
+    // storage owned by the temporary.
+    if (call_expr->isLValue() || call_expr->getType()->isPointerType()) {
+      for (const auto* arg : call_expr->arguments()) {
+        if (MayDependOnLifetimeOfMaterializedTemporaryStorage(*arg)) {
+          return true;
+        }
+      }
     }
+    return false;
   }
+
   return false;
 }
 

test/execute/materialized_temporary_2/harness.cc

@@ -0,0 +1,7 @@
+#include <iostream>
+
+int foo();
+
+int main() {
+  std::cout << foo() << std::endl;
+}

test/execute/materialized_temporary_2/mutants.txt

@@ -0,0 +1,3 @@
+0 ; Replace 42 or overall expression with 0
+1 ; Replace 42 or overall expression with 1
+-1 ; Replace overall expression with -1

test/execute/materialized_temporary_2/original.txt

@@ -0,0 +1 @@
+42

test/execute/materialized_temporary_2/tomutate.cc

@@ -0,0 +1,23 @@
+#include <optional>
+
+struct S {
+  unsigned f;
+  ~S() {
+    f = 12;
+  }
+};
+
+struct T {
+  std::optional<S> g;
+  std::optional<S> getg() const { return g; }
+};
+
+unsigned bar(std::optional<S> g) {
+  return g->f;
+}
+
+int foo() {
+    T x;
+    x.g = S{42};
+    return bar(S{x.getg()->f});
+}

test/single_file/initializer_list_with_templates.cc.expected

@@ -38,7 +38,7 @@ static bool __dredd_enabled_mutation(int local_mutation_id) {
         if (!token.empty()) {
           int value = std::stoi(token);
           int local_value = value - 0;
-          if (local_value >= 0 && local_value < 13) {
+          if (local_value >= 0 && local_value < 8) {
             enabled_bitset[local_value / 64] |= (static_cast<uint64_t>(1) << (local_value % 64));
             some_mutation_enabled = true;
           }
@@ -81,4 +81,4 @@ template <typename> class c : a {
   }
 };
 
-void f() { if (!__dredd_enabled_mutation(12)) { std::make_shared<c<int>>(__dredd_replace_expr_int_constant(__dredd_replace_expr_int_constant(2, 2), 7)); } }
+void f() { if (!__dredd_enabled_mutation(7)) { std::make_shared<c<int>>(__dredd_replace_expr_int_constant(2, 2)); } }

test/single_file/initializer_list_with_templates.cc.noopt.expected

@@ -38,7 +38,7 @@ static bool __dredd_enabled_mutation(int local_mutation_id) {
         if (!token.empty()) {
           int value = std::stoi(token);
           int local_value = value - 0;
-          if (local_value >= 0 && local_value < 19) {
+          if (local_value >= 0 && local_value < 13) {
             enabled_bitset[local_value / 64] |= (static_cast<uint64_t>(1) << (local_value % 64));
             some_mutation_enabled = true;
           }
@@ -75,4 +75,4 @@ template <typename> class c : a {
   }
 };
 
-void f() { if (!__dredd_enabled_mutation(18)) { std::make_shared<c<int>>(__dredd_replace_expr_int(__dredd_replace_expr_int(2, 6), 12)); } }
+void f() { if (!__dredd_enabled_mutation(12)) { std::make_shared<c<int>>(__dredd_replace_expr_int(2, 6)); } }