Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid redundant/equivalent unsigned comparison mutations #297

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Avoid redundant/equivalent unsigned comparison mutations #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cc2c1f0
Avoid redundant/equivalent unsigned comparison mutations
afd Jul 16, 2024
f5843e0
Fix.
afd Jul 16, 2024
4fe30c1
Fix number of spir-v mutants.
afd Jul 16, 2024
b67843c
Addressed comments from JFJJ, and renamed "operator_kind" to
afd Jul 17, 2024
b3f421e
Update expected mutants for SPIR-V.
afd Jul 17, 2024
0ba2591
Fix number of expected mutants for LLVM.
afd Jul 17, 2024
407e34e
Fix to make equivalence checks symmetric.
afd Jul 17, 2024
36c137f
Fix single file expectations.
afd Jul 17, 2024
79d0de9
Fix.
afd Jul 17, 2024
e0696df
Fix iwyu error.
afd Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/cxx_apps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=68841
EXPECTED_NUM_MUTANTS=68629
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."
Expand All @@ -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=97924
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."
Expand Down
13 changes: 9 additions & 4 deletions src/libdredd/include/libdredd/mutation_replace_binary_operator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,23 @@ class MutationReplaceBinaryOperator : public Mutation {
clang::ASTContext& ast_context) const;

[[nodiscard]] bool IsRedundantReplacementOperator(
clang::BinaryOperatorKind operator_kind,
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const;

[[nodiscard]] bool IsRedundantReplacementForBooleanValuedOperator(
clang::BinaryOperatorKind operator_kind) const;
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const;

[[nodiscard]] bool IsRedundantReplacementForUnsignedComparison(
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const;

[[nodiscard]] bool IsRedundantReplacementForArithmeticOperator(
clang::BinaryOperatorKind operator_kind,
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const;

[[nodiscard]] bool IsValidReplacementOperator(
clang::BinaryOperatorKind operator_kind) const;
clang::BinaryOperatorKind replacement_operator) const;

// Replaces binary expressions with either the left or right operand.
void GenerateArgumentReplacement(
Expand Down
4 changes: 2 additions & 2 deletions src/libdredd/include/libdredd/mutation_replace_unary_operator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ class MutationReplaceUnaryOperator : public Mutation {
[[nodiscard]] static bool IsPrefix(clang::UnaryOperatorKind operator_kind);

[[nodiscard]] bool IsValidReplacementOperator(
clang::UnaryOperatorKind operator_kind) const;
clang::UnaryOperatorKind replacement_operator) const;

[[nodiscard]] bool IsRedundantReplacementOperator(
clang::UnaryOperatorKind operator_kind,
clang::UnaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const;

[[nodiscard]] bool IsOperatorSelfInverse() const;
Expand Down
160 changes: 123 additions & 37 deletions src/libdredd/src/mutation_replace_binary_operator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@

#include <algorithm>
#include <cassert>
#include <functional>
#include <initializer_list>
#include <set>
#include <sstream>
#include <string>
#include <unordered_set>
Expand Down Expand Up @@ -51,12 +53,14 @@ MutationReplaceBinaryOperator::MutationReplaceBinaryOperator(
ast_context) {}

bool MutationReplaceBinaryOperator::IsRedundantReplacementOperator(
clang::BinaryOperatorKind operator_kind,
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const {
if (IsRedundantReplacementForBooleanValuedOperator(operator_kind)) {
if (IsRedundantReplacementForBooleanValuedOperator(replacement_operator,
ast_context)) {
return true;
}
if (IsRedundantReplacementForArithmeticOperator(operator_kind, ast_context)) {
if (IsRedundantReplacementForArithmeticOperator(replacement_operator,
ast_context)) {
return true;
}
return false;
Expand All @@ -65,7 +69,7 @@ bool MutationReplaceBinaryOperator::IsRedundantReplacementOperator(
// Certain operators such as % are not compatible with floating point numbers,
// this function checks which operators can be applied for each type.
bool MutationReplaceBinaryOperator::IsValidReplacementOperator(
clang::BinaryOperatorKind operator_kind) const {
clang::BinaryOperatorKind replacement_operator) const {
const auto* lhs_type =
binary_operator_->getLHS()->getType()->getAs<clang::BuiltinType>();
assert((lhs_type->isFloatingPoint() || lhs_type->isInteger()) &&
Expand All @@ -75,13 +79,13 @@ bool MutationReplaceBinaryOperator::IsValidReplacementOperator(
assert((rhs_type->isFloatingPoint() || rhs_type->isInteger()) &&
"Expected rhs to be either integer or floating-point.");
return !((lhs_type->isFloatingPoint() || rhs_type->isFloatingPoint()) &&
(operator_kind == clang::BO_Rem ||
operator_kind == clang::BO_AndAssign ||
operator_kind == clang::BO_OrAssign ||
operator_kind == clang::BO_RemAssign ||
operator_kind == clang::BO_ShlAssign ||
operator_kind == clang::BO_ShrAssign ||
operator_kind == clang::BO_XorAssign));
(replacement_operator == clang::BO_Rem ||
replacement_operator == clang::BO_AndAssign ||
replacement_operator == clang::BO_OrAssign ||
replacement_operator == clang::BO_RemAssign ||
replacement_operator == clang::BO_ShlAssign ||
replacement_operator == clang::BO_ShrAssign ||
replacement_operator == clang::BO_XorAssign));
}

std::string MutationReplaceBinaryOperator::GetFunctionName(
Expand Down Expand Up @@ -346,16 +350,17 @@ void MutationReplaceBinaryOperator::GenerateBinaryOperatorReplacement(
bool only_track_mutant_coverage, int mutation_id_base,
std::stringstream& new_function, int& mutation_id_offset,
protobufs::MutationReplaceBinaryOperator& protobuf_message) const {
for (auto operator_kind :
for (auto replacement_operator :
GetReplacementOperators(optimise_mutations, ast_context)) {
if (!only_track_mutant_coverage) {
new_function << " if (__dredd_enabled_mutation(local_mutation_id + "
<< mutation_id_offset << ")) return " << arg1_evaluated
<< " "
<< clang::BinaryOperator::getOpcodeStr(operator_kind).str()
<< " " << arg2_evaluated << ";\n";
new_function
<< " if (__dredd_enabled_mutation(local_mutation_id + "
<< mutation_id_offset << ")) return " << arg1_evaluated << " "
<< clang::BinaryOperator::getOpcodeStr(replacement_operator).str()
<< " " << arg2_evaluated << ";\n";
}
AddMutationInstance(mutation_id_base, OperatorKindToAction(operator_kind),
AddMutationInstance(mutation_id_base,
OperatorKindToAction(replacement_operator),
mutation_id_offset, protobuf_message);
}
}
Expand Down Expand Up @@ -1024,34 +1029,108 @@ MutationReplaceBinaryOperator::ClangOperatorKindToProtobufOperatorKind(

bool MutationReplaceBinaryOperator::
IsRedundantReplacementForBooleanValuedOperator(
clang::BinaryOperatorKind operator_kind) const {
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const {
if (IsRedundantReplacementForUnsignedComparison(replacement_operator,
ast_context)) {
return true;
}

switch (binary_operator_->getOpcode()) {
// From
// https://people.cs.umass.edu/~rjust/publ/non_redundant_mutants_jstvr_2014.pdf:
// For boolean operators, only a subset of replacements are non-redundant.
case clang::BO_LAnd:
return operator_kind != clang::BO_EQ;
return replacement_operator != clang::BO_EQ;
case clang::BO_LOr:
return operator_kind != clang::BO_NE;
return replacement_operator != clang::BO_NE;
case clang::BO_GT:
return operator_kind != clang::BO_GE && operator_kind != clang::BO_NE;
return replacement_operator != clang::BO_GE &&
replacement_operator != clang::BO_NE;
case clang::BO_GE:
return operator_kind != clang::BO_GT && operator_kind != clang::BO_EQ;
return replacement_operator != clang::BO_GT &&
replacement_operator != clang::BO_EQ;
case clang::BO_LT:
return operator_kind != clang::BO_LE && operator_kind != clang::BO_NE;
return replacement_operator != clang::BO_LE &&
replacement_operator != clang::BO_NE;
case clang::BO_LE:
return operator_kind != clang::BO_LT && operator_kind != clang::BO_EQ;
return replacement_operator != clang::BO_LT &&
replacement_operator != clang::BO_EQ;
case clang::BO_EQ:
return operator_kind != clang::BO_LE && operator_kind != clang::BO_GE;
return replacement_operator != clang::BO_LE &&
replacement_operator != clang::BO_GE;
case clang::BO_NE:
return operator_kind != clang::BO_LT && operator_kind != clang::BO_GT;
return replacement_operator != clang::BO_LT &&
replacement_operator != clang::BO_GT;
default:
return false;
}
}

bool MutationReplaceBinaryOperator::IsRedundantReplacementForUnsignedComparison(
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const {
if (!binary_operator_->getLHS()->getType()->isUnsignedIntegerType() ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) is it possible for LHS and RHS to have different signedness ? I have thought that one side will be wrapped under implicit cast to match the type, such that we can always sure both side will have same type(and thus same signedness) at this point.
(2) I would suggest checking for signedness in the caller(ie: IsRedundantReplacementForBooleanValuedOperator) before calling a function named :IsRedundantReplacementForUnsignedComparison.

!binary_operator_->getRHS()->getType()->isUnsignedIntegerType()) {
return false;
}
if (MutationReplaceExpr::ExprIsEquivalentToInt(*binary_operator_->getLHS(), 0,
ast_context)) {
// LHS is 0
if (replacement_operator == clang::BO_GT ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a room for simplification here. See the corresponding comment for RHS is 0

replacement_operator == clang::BO_LE) {
// We are considering mutating an expression of the form "0 op a" to
// one of:
//
// - "0 > a"
// - "0 <= a"
//
// These are subsumed by replacing the overall expression with "false"
// and "true", respectively.
return true;
}
if (std::set({binary_operator_->getOpcode(), replacement_operator}) ==
std::set<clang::BinaryOperatorKind>({clang::BO_EQ, clang::BO_GE})) {
// "0 == a" is equivalent to "0 >= a"
return true;
}
Comment on lines +1092 to +1096
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the fact that 0 == a is equivalent to 0 >= a and commutativity, it is also redundant to replace 0 >= a by 0 == a for signed integers (which is unlikely but may occur). I think equality on ordered sets compare element wise so this wouldn't currently cover the second case.

if (std::set({binary_operator_->getOpcode(), replacement_operator}) ==
std::set<clang::BinaryOperatorKind>({clang::BO_NE, clang::BO_LT})) {
// "0 != a" equivalent to "0 < a"
return true;
}
}
Comment on lines +1097 to +1102
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, it is also redundant to replace 0 < a with 0 != a

if (MutationReplaceExpr::ExprIsEquivalentToInt(*binary_operator_->getRHS(), 0,
ast_context)) {
// RHS is 0
Copy link
Collaborator

@JonathanFoo0523 JonathanFoo0523 Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hypothesize that everything under this if-statement can be made more succinct with the following checking logic

if binary_operator_->getOpcode() in ['<' '>=']:
  return replacement_operator not in ['!=', '==']
return True

if (replacement_operator == clang::BO_LT ||
replacement_operator == clang::BO_GE) {
// We are considering mutating an expression of the form "a op 0" to
// one of:
//
// - "a < 0"
// - "a >= 0"
//
// These are subsumed by replacing the overall expression with "false"
// and "true", respectively.
return true;
}
if (binary_operator_->getOpcode() == clang::BO_EQ &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above you used set equality but here used two equality checks, is there any reason?

replacement_operator == clang::BO_LE) {
// "a == 0" is equivalent to "a <= 0"
return true;
}
Comment on lines +1118 to +1122
Copy link
Collaborator

@JamesLee-Jones JamesLee-Jones Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, it is also redundant to replace a <= 0 with 0 == a.

if (binary_operator_->getOpcode() == clang::BO_NE &&
replacement_operator == clang::BO_GT) {
Comment on lines +1123 to +1124
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above you used set equality but here used two equality checks, is there any reason?

// "a != 0" equivalent to "a > 0"
return true;
Comment on lines +1123 to +1126
Copy link
Collaborator

@JamesLee-Jones JamesLee-Jones Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, it is also redundant to replace a > 0 with a != 0.

}
}
return false;
}

bool MutationReplaceBinaryOperator::IsRedundantReplacementForArithmeticOperator(
clang::BinaryOperatorKind operator_kind,
clang::BinaryOperatorKind replacement_operator,
const clang::ASTContext& ast_context) const {
// In the case where both operands are 0, the only case that isn't covered
// by constant replacement is undefined behaviour, this is achieved by /.
Expand All @@ -1063,7 +1142,7 @@ bool MutationReplaceBinaryOperator::IsRedundantReplacementForArithmeticOperator(
0, ast_context) ||
MutationReplaceExpr::ExprIsEquivalentToFloat(*binary_operator_->getRHS(),
0.0, ast_context))) {
if (operator_kind == clang::BO_Div) {
if (replacement_operator == clang::BO_Div) {
return false;
}
}
Expand All @@ -1078,9 +1157,12 @@ bool MutationReplaceBinaryOperator::IsRedundantReplacementForArithmeticOperator(
// replacement with the right operand; * is equivalent to replacement with
// the constant 0 and % is equivalent to replacement with / in that both
// cases lead to undefined behaviour.
if (operator_kind == clang::BO_Add || operator_kind == clang::BO_Sub ||
operator_kind == clang::BO_Shl || operator_kind == clang::BO_Shr ||
operator_kind == clang::BO_Mul || operator_kind == clang::BO_Rem) {
if (replacement_operator == clang::BO_Add ||
replacement_operator == clang::BO_Sub ||
replacement_operator == clang::BO_Shl ||
replacement_operator == clang::BO_Shr ||
replacement_operator == clang::BO_Mul ||
replacement_operator == clang::BO_Rem) {
return true;
}
}
Expand All @@ -1091,7 +1173,8 @@ bool MutationReplaceBinaryOperator::IsRedundantReplacementForArithmeticOperator(
1.0, ast_context))) {
// When the right operand is 1: * and / are equivalent to replacement by
// the left operand.
if (operator_kind == clang::BO_Mul || operator_kind == clang::BO_Div) {
if (replacement_operator == clang::BO_Mul ||
replacement_operator == clang::BO_Div) {
return true;
}
}
Expand All @@ -1103,9 +1186,12 @@ bool MutationReplaceBinaryOperator::IsRedundantReplacementForArithmeticOperator(
// When the left operand is 0: *, /, %, << and >> are equivalent to
// replacement by the constant 0 and + is equivalent to replacement by the
// right operand.
if (operator_kind == clang::BO_Add || operator_kind == clang::BO_Shl ||
operator_kind == clang::BO_Shr || operator_kind == clang::BO_Mul ||
operator_kind == clang::BO_Rem || operator_kind == clang::BO_Div) {
if (replacement_operator == clang::BO_Add ||
replacement_operator == clang::BO_Shl ||
replacement_operator == clang::BO_Shr ||
replacement_operator == clang::BO_Mul ||
replacement_operator == clang::BO_Rem ||
replacement_operator == clang::BO_Div) {
return true;
}
}
Expand All @@ -1114,7 +1200,7 @@ bool MutationReplaceBinaryOperator::IsRedundantReplacementForArithmeticOperator(
1, ast_context) ||
MutationReplaceExpr::ExprIsEquivalentToFloat(*binary_operator_->getLHS(),
1.0, ast_context)) &&
operator_kind == clang::BO_Mul) {
replacement_operator == clang::BO_Mul) {
// When the left operand is 1: * is equivalent to replacement by the right
// operand.
return true;
Expand Down
Loading
Loading