Avoid redundant/equivalent unsigned comparison mutations #297

afd · 2024-07-16T16:39:34Z

Fixes #240.

JamesLee-Jones

Looks good! It may be worth copying the test case added to C++ as we have done elsewhere in case the functionality diverges in the future (although unlikely).

JonathanFoo0523

I think the role of candidate mutation and the binary operator to apply mutation ion is switched on IsRedundantReplacementForUnsignedComparison, and as a result, subsumed mutation are produced. I might be wrong tho. Let me know if I am wrong, I will have another look.

Does it work if we don't have an unsigned literal(eg: x !=0 instead of x != 0u)?

test/single_file/unsigned_comparisons_with_zero.c.expected

src/libdredd/src/mutation_replace_binary_operator.cc

test/single_file/unsigned_comparisons_with_zero.c.expected

afd · 2024-07-17T07:06:25Z

@JamesLee-Jones Thanks for the comment. The C++ single file tests take considerably longer to run compared with the C ones, so I have decided not to add them in this instance, feeling confident this won't change.

afd · 2024-07-17T07:43:21Z

@JonathanFoo0523 - the optimisation does work if 0u is replaced by 0 - in next iteration I have added tests to check for this.

It raises room for another optimisation, though, which is that when an int is cast (implicitly or otherwise) to unsigned, we mutate both before and after the cast, which will lead to replacement with 0 and 1 occurring redundantly as they will be used in both places. I'll open an issue for this.

afd · 2024-07-17T07:46:02Z

@JonathanFoo0523 Thanks for your comments - good spots. I have revised - and to make it less likely that I'll make a mistake like this again I renamed "operator_kind" to "replacement_operator" in all of the functions that are considering whether a replacement is legitimate (both for unary and binary operator mutations).

afd · 2024-07-17T09:24:04Z

It raises room for another optimisation, though, which is that when an int is cast (implicitly or otherwise) to unsigned, we mutate both before and after the cast, which will lead to replacement with 0 and 1 occurring redundantly as they will be used in both places. I'll open an issue for this.

Actually I think I got this wrong - perhaps I looked at the non-optimised code.

JonathanFoo0523 · 2024-07-17T14:23:21Z

test/single_file/unsigned_comparisons_with_zero.c.expected

+
+static int __dredd_replace_binary_operator_LE_arg1_unsigned_int_arg2_unsigned_int_rhs_zero(unsigned int arg1, unsigned int arg2, int local_mutation_id) {
+  if (!__dredd_some_mutation_enabled) return arg1 <= arg2;
+  if (__dredd_enabled_mutation(local_mutation_id + 0)) return arg1 == arg2;


Something might not be right here, as

a <= 0 is equivalent to a == 0

Good catch - I have made the equivalence check symmetric now.

+----+------+------+-------+ | a | a<=0 | TRUE | FALSE | +----+------+------+-------+ | 0 | T | T | F | +----+------+------+-------+ | >0 | F | T | F | +----+------+------+-------+

Still persist. I would say we shouldnt have mutation in this function

JonathanFoo0523 · 2024-07-17T14:24:59Z

test/single_file/unsigned_comparisons_with_zero.c.expected

+
+static int __dredd_replace_binary_operator_GT_arg1_unsigned_int_arg2_unsigned_int_rhs_zero(unsigned int arg1, unsigned int arg2, int local_mutation_id) {
+  if (!__dredd_some_mutation_enabled) return arg1 > arg2;
+  if (__dredd_enabled_mutation(local_mutation_id + 0)) return arg1 != arg2;


something wrong

a > 0 is equivalent to a != 0

Good catch - I have made the equivalence check symmetric now.

still persist

+----+-----+------+-------+ | a | a>0 | FALSE| TRUE | +----+-----+------+-------+ | 0 | F | F | T | +----+-----+------+-------+ | >0 | T | F | T | +----+-----+------+-------+

I would say we shouldnt have mutation in this function

JonathanFoo0523

Didn't look through the source code as I think the produced expected testcase doesn't look right. I will re-review the code after.

test/single_file/unsigned_comparisons_with_zero.c.expected

Fixes #240.

"replacement_operator" wherever appropriate in unary and binary operator mutations, to improve readability.

afd · 2024-07-18T10:04:39Z

@JamesLee-Jones would you be able to take a look again please?

JamesLee-Jones

I believe there are a couple of cases that have been missed and a couple of small questions but otherwise looks good!

JamesLee-Jones · 2024-07-18T10:16:10Z

src/libdredd/src/mutation_replace_binary_operator.cc

+    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;
+    }


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.

JamesLee-Jones · 2024-07-18T10:16:41Z

src/libdredd/src/mutation_replace_binary_operator.cc

+    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;
+    }
+  }


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

JamesLee-Jones · 2024-07-18T10:17:20Z

src/libdredd/src/mutation_replace_binary_operator.cc

+    if (binary_operator_->getOpcode() == clang::BO_EQ &&
+        replacement_operator == clang::BO_LE) {
+      // "a == 0" is equivalent to "a <= 0"
+      return true;
+    }


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

JamesLee-Jones · 2024-07-18T10:17:32Z

src/libdredd/src/mutation_replace_binary_operator.cc

+    if (binary_operator_->getOpcode() == clang::BO_NE &&
+        replacement_operator == clang::BO_GT) {
+      // "a != 0" equivalent to "a > 0"
+      return true;


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

JamesLee-Jones · 2024-07-18T10:19:10Z

src/libdredd/src/mutation_replace_binary_operator.cc

+      // and "true", respectively.
+      return true;
+    }
+    if (binary_operator_->getOpcode() == clang::BO_EQ &&


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

JamesLee-Jones · 2024-07-18T10:19:23Z

src/libdredd/src/mutation_replace_binary_operator.cc

+    if (binary_operator_->getOpcode() == clang::BO_NE &&
+        replacement_operator == clang::BO_GT) {


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

JonathanFoo0523 · 2024-07-18T11:09:15Z

src/libdredd/src/mutation_replace_binary_operator.cc

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


(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.

test/single_file/unsigned_comparisons_with_zero.c.expected

JonathanFoo0523 · 2024-07-18T12:25:23Z

src/libdredd/src/mutation_replace_binary_operator.cc

+  }
+  if (MutationReplaceExpr::ExprIsEquivalentToInt(*binary_operator_->getRHS(), 0,
+                                                 ast_context)) {
+    // RHS is 0


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

JonathanFoo0523 · 2024-07-18T12:27:48Z

src/libdredd/src/mutation_replace_binary_operator.cc

+  if (MutationReplaceExpr::ExprIsEquivalentToInt(*binary_operator_->getLHS(), 0,
+                                                 ast_context)) {
+    // LHS is 0
+    if (replacement_operator == clang::BO_GT ||


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

afd requested review from JonathanFoo0523 and JamesLee-Jones July 16, 2024 16:39

JamesLee-Jones approved these changes Jul 16, 2024

View reviewed changes

JonathanFoo0523 reviewed Jul 16, 2024

View reviewed changes

afd requested a review from JonathanFoo0523 July 17, 2024 07:44

JonathanFoo0523 reviewed Jul 17, 2024

View reviewed changes

test/single_file/unsigned_comparisons_with_zero.c.expected Show resolved Hide resolved

afd force-pushed the issue-240 branch from c3add77 to 64feb80 Compare July 17, 2024 14:51

afd added 8 commits July 17, 2024 17:03

Avoid redundant/equivalent unsigned comparison mutations

cc2c1f0

Fixes #240.

Fix.

f5843e0

Fix number of spir-v mutants.

4fe30c1

Addressed comments from JFJJ, and renamed "operator_kind" to

b67843c

"replacement_operator" wherever appropriate in unary and binary operator mutations, to improve readability.

Update expected mutants for SPIR-V.

b3f421e

Fix number of expected mutants for LLVM.

0ba2591

Fix to make equivalence checks symmetric.

407e34e

Fix single file expectations.

36c137f

afd force-pushed the issue-240 branch from 64feb80 to 36c137f Compare July 17, 2024 16:47

afd added 2 commits July 17, 2024 20:16

Fix.

79d0de9

Fix iwyu error.

e0696df

afd mentioned this pull request Jul 18, 2024

Consider different non-redundant ROR mutants #303

Open

afd requested a review from JonathanFoo0523 July 18, 2024 10:04

JamesLee-Jones requested changes Jul 18, 2024

View reviewed changes

JonathanFoo0523 reviewed Jul 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid redundant/equivalent unsigned comparison mutations #297

Avoid redundant/equivalent unsigned comparison mutations #297

afd commented Jul 16, 2024

JamesLee-Jones left a comment

JonathanFoo0523 left a comment

afd commented Jul 17, 2024

afd commented Jul 17, 2024

afd commented Jul 17, 2024

afd commented Jul 17, 2024

JonathanFoo0523 Jul 17, 2024

afd Jul 17, 2024

JonathanFoo0523 Jul 18, 2024

JonathanFoo0523 Jul 17, 2024

afd Jul 17, 2024

JonathanFoo0523 Jul 18, 2024 •

edited

Loading

JonathanFoo0523 left a comment

afd commented Jul 18, 2024

JamesLee-Jones left a comment

JamesLee-Jones Jul 18, 2024

JamesLee-Jones Jul 18, 2024

JamesLee-Jones Jul 18, 2024 •

edited

Loading

JamesLee-Jones Jul 18, 2024 •

edited

Loading

JamesLee-Jones Jul 18, 2024

JamesLee-Jones Jul 18, 2024

JonathanFoo0523 Jul 18, 2024

JonathanFoo0523 Jul 18, 2024 •

edited

Loading

JonathanFoo0523 Jul 18, 2024

		if (binary_operator_->getOpcode() == clang::BO_NE &&
		replacement_operator == clang::BO_GT) {

Avoid redundant/equivalent unsigned comparison mutations #297

Are you sure you want to change the base?

Avoid redundant/equivalent unsigned comparison mutations #297

Conversation

afd commented Jul 16, 2024

JamesLee-Jones left a comment

Choose a reason for hiding this comment

JonathanFoo0523 left a comment

Choose a reason for hiding this comment

afd commented Jul 17, 2024

afd commented Jul 17, 2024

afd commented Jul 17, 2024

afd commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanFoo0523 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

JonathanFoo0523 left a comment

Choose a reason for hiding this comment

afd commented Jul 18, 2024

JamesLee-Jones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesLee-Jones Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

JamesLee-Jones Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanFoo0523 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanFoo0523 Jul 18, 2024 •

edited

Loading

JamesLee-Jones Jul 18, 2024 •

edited

Loading

JamesLee-Jones Jul 18, 2024 •

edited

Loading

JonathanFoo0523 Jul 18, 2024 •

edited

Loading