From b5b300dbefdbbb005ce030f6a111fa42b08d5fe6 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 24 Sep 2024 22:12:00 +0100 Subject: [PATCH 01/12] DeadCode: Use HoldsForAllInstances Eliminate false positives where a line of code is used in some copies (instances) but not others. --- .../cpp/rules/deadcode/DeadCode.qll | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll index 2b5be15e80..1ccfb91095 100644 --- a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll +++ b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll @@ -12,6 +12,7 @@ */ import cpp +import codingstandards.cpp.alertreporting.HoldsForAllInstances import codingstandards.cpp.Customizations import codingstandards.cpp.Exclusions import codingstandards.cpp.deadcode.UselessAssignments @@ -31,10 +32,6 @@ predicate isDeadOrUnreachableStmt(Stmt s) { s.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock() } -/** - * Holds if the `Stmt` `s` is dead, i.e. could be executed, but its removal would not meaningfully - * affect the program. - */ predicate isDeadStmt(Stmt s) { // A `DeclStmt` is dead code if: // - All the declarations are variable declarations @@ -108,17 +105,33 @@ predicate isDeadStmt(Stmt s) { exists(TryStmt ts | s = ts and isDeadStmt(ts.getStmt())) } -query predicate problems(Stmt s, string message) { - not isExcluded(s, getQuery()) and +/** + * Holds if the `Stmt` `s` is dead, i.e. could be executed, but its removal would not meaningfully + * affect the program. + */ +class DeadStmtInstance extends Stmt { + DeadStmtInstance() { + isDeadStmt(this) and + // Exclude compiler generated statements + not this.isCompilerGenerated() and + // Exclude code fully generated by macros, because the code may be "live" in other expansions + not this.isInMacroExpansion() and + // MISRA defines dead code as an "_executed_ statement whose removal would not affect the program + // output". We therefore exclude unreachable statements as they are, by definition, not executed. + not this.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock() + } +} + +class DeadStmt = HoldsForAllInstances::LogicalResultStmt; + +query predicate problems(DeadStmt s, string message) { + not isExcluded(s.getAStmtInstance(), getQuery()) and message = "This statement is dead code." and - isDeadStmt(s) and // Report only the highest level dead statement, to avoid over reporting - not isDeadStmt(s.getParentStmt()) and - // MISRA defines dead code as an "_executed_ statement whose removal would not affect the program - // output". We therefore exclude unreachable statements as they are, by definition, not executed. - not s.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock() and - // Exclude code fully generated by macros, because the code may be "live" in other expansions - not s.isInMacroExpansion() and - // Exclude compiler generated statements - not s.isCompilerGenerated() + not exists(DeadStmt parent | + // All instances must share a dead statement parent for us to report the parent instead + forall(Stmt instance | instance = s.getAStmtInstance() | + parent.getAStmtInstance() = instance.getParentStmt() + ) + ) } From efa556c0e33d7a24e482b237df09a8d0563e66d2 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 24 Sep 2024 23:25:44 +0100 Subject: [PATCH 02/12] Add library for reporting if all instances hold Add a utility library for determing whether a condition holds for all copies of a statement in a program. --- .../alertreporting/HoldsForAllInstances.qll | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll new file mode 100644 index 0000000000..fcf307c975 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll @@ -0,0 +1,106 @@ +/** + * A module for considering whether a result occurs in all instances (e.g. copies) of the code at a + * given location. + * + * Multiple instances of a statement at the same location can occur for two main reasons: + * 1. Instantiations of a template + * 2. Re-compilation of a file under a different context + * This module helps ensure that a particular condition holds for all copies of a particular logical + * statement. For example, this can be used to determine whether a line of code is dead in all copies + * of a piece of code. + * + * This module is parameterized by a set of _candidate_ statements in the program. For each candidate + * statement, we determine whether all other statements that occur at the same location in the + * program are also part of the same set, ignoring any results generated by macros. + * + * We do so by reporting a new type of result, `LogicalResultStmt`, which represents a logical result + * where all instances of a statement at a given location are considered to be part of the same set. + */ + +import cpp + +/** + * Holds if the `Stmt` `s` is not within a macro expansion, i.e. generated by a macro, but not the + * outermost `Stmt` generated by the macro. + */ +predicate isNotWithinMacroExpansion(Stmt s) { + not s.isInMacroExpansion() + or + exists(MacroInvocation mi | + mi.getStmt() = s and + not exists(mi.getParentInvocation()) + ) +} + +/** A candidate set of types. */ +signature class CandidateStmtSig extends Stmt; + +/** + * A module for considering whether a result occurs in all instances (e.g. copies) of the code at a + * given location. + */ +module HoldsForAllInstances { + private predicate hasLocation( + Stmt s, string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + s.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } + + final private class MyStmt = Stmt; + + /** + * A `Stmt` that appears at the same location as a candidate statement. + */ + private class RelevantStmt extends MyStmt { + CandidateStmt s; + + RelevantStmt() { + exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | + hasLocation(this, filepath, startline, startcolumn, endline, endcolumn) and + hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) + ) and + // Not within a macro expansion, as we cannot match up instances by location in that + // case + isNotWithinMacroExpansion(this) and + // Ignore catch handlers, as they occur at the same location as the catch block + not this instanceof Handler + } + + CandidateStmt getCandidateStmt() { result = s } + } + + newtype TResultStmts = + TLogicalResultStmt(string filepath, int startline, int startcolumn, int endline, int endcolumn) { + exists(CandidateStmt s | + // Only consider candidates where we can match up the location + isNotWithinMacroExpansion(s) and + hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) and + // All relevant statements that occur at the same location are candidates + forex(RelevantStmt relevantStmt | s = relevantStmt.getCandidateStmt() | + relevantStmt instanceof CandidateStmt + ) + ) + } + + /** + * A logical result statement, representing all instances of a statement that occur at the same + * location. + */ + class LogicalResultStmt extends TLogicalResultStmt { + predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + this = TLogicalResultStmt(filepath, startline, startcolumn, endline, endcolumn) + } + + /** Gets an instance of this logical result statement. */ + CandidateStmt getAStmtInstance() { + exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | + this = TLogicalResultStmt(filepath, startline, startcolumn, endline, endcolumn) and + hasLocation(result, filepath, startline, startcolumn, endline, endcolumn) + ) + } + + string toString() { result = getAStmtInstance().toString() } + } +} From 61cd6ca2c3d79d1ac90d53f25ca863fb6fe0a755 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 25 Sep 2024 09:55:09 +0100 Subject: [PATCH 03/12] Consistently exclude macro generated statements --- cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll index 1ccfb91095..667c9020a9 100644 --- a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll +++ b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll @@ -115,7 +115,7 @@ class DeadStmtInstance extends Stmt { // Exclude compiler generated statements not this.isCompilerGenerated() and // Exclude code fully generated by macros, because the code may be "live" in other expansions - not this.isInMacroExpansion() and + isNotWithinMacroExpansion(this) and // MISRA defines dead code as an "_executed_ statement whose removal would not affect the program // output". We therefore exclude unreachable statements as they are, by definition, not executed. not this.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock() From aca6d40da65a1158441363943198d6da6a15bc22 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 25 Sep 2024 10:04:10 +0100 Subject: [PATCH 04/12] Fix #604 - add test of template results The modifications to the query to handle multiple copies of a statement across different targets also support reporting of issues across multiple template instantiations. This commit adds additional tests to demonstrate that this works effectively. --- .../test/rules/deadcode/DeadCode.expected | 2 ++ cpp/common/test/rules/deadcode/test.cpp | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/cpp/common/test/rules/deadcode/DeadCode.expected b/cpp/common/test/rules/deadcode/DeadCode.expected index aec93e0914..bc0afc7c60 100644 --- a/cpp/common/test/rules/deadcode/DeadCode.expected +++ b/cpp/common/test/rules/deadcode/DeadCode.expected @@ -15,3 +15,5 @@ | test.cpp:85:3:85:43 | declaration | This statement is dead code. | | test.cpp:87:3:87:30 | declaration | This statement is dead code. | | test.cpp:90:3:90:50 | declaration | This statement is dead code. | +| test.cpp:108:3:108:21 | ExprStmt | This statement is dead code. | +| test.cpp:120:3:120:23 | ExprStmt | This statement is dead code. | diff --git a/cpp/common/test/rules/deadcode/test.cpp b/cpp/common/test/rules/deadcode/test.cpp index d9c0cab277..7632310e1c 100644 --- a/cpp/common/test/rules/deadcode/test.cpp +++ b/cpp/common/test/rules/deadcode/test.cpp @@ -91,3 +91,31 @@ int test_dead_code(int x) { return live5 + live6 + constexpr_used_array[1]; // COMPLIANT } + +class Foo { +public: + void bar() { may_have_side_effects(); } +}; + +class Baz { +public: + void bar() {} // No side effects +}; + +template void test_template() { + T t; + t.bar(); // COMPLIANT + no_side_effects(1); // NON_COMPLIANT +} + +template void test_unused_template() { + T t; + t.bar(); // COMPLIANT + no_side_effects( + 1); // NON_COMPLIANT[FALSE_NEGATIVE] - unused templates are not extracted +} + +void test() { + test_template(); + test_template(); // NON_COMPLIANT - template call has no affect +} \ No newline at end of file From 4d457b8084a885318230dc4cb773eece357e0087 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 25 Sep 2024 10:22:06 +0100 Subject: [PATCH 05/12] DeadCode: Add macro/template tests Ensure that macro expansions and multiple instances of code work together. --- .../test/rules/deadcode/DeadCode.expected | 6 +++-- cpp/common/test/rules/deadcode/test.cpp | 26 ++++++++++++++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/cpp/common/test/rules/deadcode/DeadCode.expected b/cpp/common/test/rules/deadcode/DeadCode.expected index bc0afc7c60..1756231343 100644 --- a/cpp/common/test/rules/deadcode/DeadCode.expected +++ b/cpp/common/test/rules/deadcode/DeadCode.expected @@ -15,5 +15,7 @@ | test.cpp:85:3:85:43 | declaration | This statement is dead code. | | test.cpp:87:3:87:30 | declaration | This statement is dead code. | | test.cpp:90:3:90:50 | declaration | This statement is dead code. | -| test.cpp:108:3:108:21 | ExprStmt | This statement is dead code. | -| test.cpp:120:3:120:23 | ExprStmt | This statement is dead code. | +| test.cpp:116:3:116:21 | ExprStmt | This statement is dead code. | +| test.cpp:117:3:117:27 | ExprStmt | This statement is dead code. | +| test.cpp:118:7:118:32 | ExprStmt | This statement is dead code. | +| test.cpp:139:3:139:35 | ExprStmt | This statement is dead code. | diff --git a/cpp/common/test/rules/deadcode/test.cpp b/cpp/common/test/rules/deadcode/test.cpp index 7632310e1c..d40667539d 100644 --- a/cpp/common/test/rules/deadcode/test.cpp +++ b/cpp/common/test/rules/deadcode/test.cpp @@ -102,10 +102,27 @@ class Baz { void bar() {} // No side effects }; +#define FULL_STMT_NO_SIDE_EFFECTS no_side_effects(1); +#define PART_STMT_NO_SIDE_EFFECTS no_side_effects(1) +#define BLOCK_SOME_SIDE_EFFECTS \ + { \ + may_have_side_effects(); \ + no_side_effects(1); \ + } + template void test_template() { T t; - t.bar(); // COMPLIANT - no_side_effects(1); // NON_COMPLIANT + t.bar(); // COMPLIANT + no_side_effects(1); // NON_COMPLIANT + FULL_STMT_NO_SIDE_EFFECTS // NON_COMPLIANT + PART_STMT_NO_SIDE_EFFECTS; // NON_COMPLIANT + BLOCK_SOME_SIDE_EFFECTS; // COMPLIANT - cannot determine loc for + // no_side_effects(1) +} + +template void test_variant_side_effects() { + T t; + t.bar(); // COMPLIANT - not dead in at least one instance } template void test_unused_template() { @@ -117,5 +134,8 @@ template void test_unused_template() { void test() { test_template(); - test_template(); // NON_COMPLIANT - template call has no affect + test_template(); + test_variant_side_effects(); // COMPLIANT + test_variant_side_effects(); // NON_COMPLIANT - no effect in this + // instantiation } \ No newline at end of file From 783f8d47cdc730db0f05f75568ef808a1967a8c0 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 25 Sep 2024 22:57:25 +0100 Subject: [PATCH 06/12] RULE-2-2: Refactor to detect dead operations The rule description for this rule in fact talks about dead operations, not dead statements. Therefore: - Unshare the rule from MISRA C++ 2008 - Implement dead operation, as per the rule --- c/misra/src/rules/RULE-2-2/DeadCode.ql | 60 ++++++++++++++++++- c/misra/test/rules/RULE-2-2/DeadCode.expected | 7 +++ c/misra/test/rules/RULE-2-2/DeadCode.qlref | 1 + c/misra/test/rules/RULE-2-2/DeadCode.testref | 1 - c/misra/test/rules/RULE-2-2/test.c | 30 ++++++++++ rule_packages/c/DeadCode.json | 16 +---- 6 files changed, 96 insertions(+), 19 deletions(-) create mode 100644 c/misra/test/rules/RULE-2-2/DeadCode.expected create mode 100644 c/misra/test/rules/RULE-2-2/DeadCode.qlref delete mode 100644 c/misra/test/rules/RULE-2-2/DeadCode.testref create mode 100644 c/misra/test/rules/RULE-2-2/test.c diff --git a/c/misra/src/rules/RULE-2-2/DeadCode.ql b/c/misra/src/rules/RULE-2-2/DeadCode.ql index c9ecb5e934..79f69e760d 100644 --- a/c/misra/src/rules/RULE-2-2/DeadCode.ql +++ b/c/misra/src/rules/RULE-2-2/DeadCode.ql @@ -14,8 +14,62 @@ import cpp import codingstandards.c.misra -import codingstandards.cpp.rules.deadcode.DeadCode +import codingstandards.cpp.deadcode.UselessAssignments -class MisraCDeadCodeQuery extends DeadCodeSharedQuery { - MisraCDeadCodeQuery() { this = DeadCodePackage::deadCodeQuery() } +/** + * Gets an explicit cast from `e` if one exists. + */ +Cast getExplicitCast(Expr e) { + exists(Conversion c | c = e.getExplicitlyConverted() | + result = c + or + result = c.(ParenthesisExpr).getExpr() + ) +} + +class ExprStmtExpr extends Expr { + ExprStmtExpr() { exists(ExprStmt es | es.getExpr() = this) } } + +/** + * An "operation" as defined by MISRA C Rule 2.2 that is dead, i.e. it's removal has no effect on + * the behaviour of the program. + */ +class DeadOperation extends Expr { + string description; + + DeadOperation() { + exists(ExprStmtExpr e | + if exists(getExplicitCast(e)) + then + this = getExplicitCast(e) and + // void conversions are permitted + not getExplicitCast(e) instanceof VoidConversion and + description = "Cast operation is unused" + else ( + this = e and + ( + if e instanceof Assignment + then + exists(SsaDefinition sd, LocalScopeVariable v | + e = sd.getDefinition() and + sd.getDefiningValue(v).isPure() and + // The definition is useless + isUselessSsaDefinition(sd, v) and + description = "Assignment to " + v.getName() + " is unused and has no side effects" + ) + else ( + e.isPure() and + description = "Result of operation is unused and has no side effects" + ) + ) + ) + ) + } + + string getDescription() { result = description } +} + +from DeadOperation deadOperation +where not isExcluded(deadOperation, DeadCodePackage::deadCodeQuery()) +select deadOperation, deadOperation.getDescription() + "." diff --git a/c/misra/test/rules/RULE-2-2/DeadCode.expected b/c/misra/test/rules/RULE-2-2/DeadCode.expected new file mode 100644 index 0000000000..6cb0fb0c60 --- /dev/null +++ b/c/misra/test/rules/RULE-2-2/DeadCode.expected @@ -0,0 +1,7 @@ +| test.c:15:3:15:11 | ... = ... | Assignment to dead1 is unused and has no side effects. | +| test.c:16:3:16:11 | ... = ... | Assignment to dead2 is unused and has no side effects. | +| test.c:19:3:19:7 | ... + ... | Result of operation is unused and has no side effects. | +| test.c:21:3:21:17 | call to no_side_effects | Result of operation is unused and has no side effects. | +| test.c:23:3:23:30 | (int)... | Cast operation is unused. | +| test.c:24:3:24:25 | (int)... | Cast operation is unused. | +| test.c:27:4:27:18 | call to no_side_effects | Result of operation is unused and has no side effects. | diff --git a/c/misra/test/rules/RULE-2-2/DeadCode.qlref b/c/misra/test/rules/RULE-2-2/DeadCode.qlref new file mode 100644 index 0000000000..761e04d51b --- /dev/null +++ b/c/misra/test/rules/RULE-2-2/DeadCode.qlref @@ -0,0 +1 @@ +rules/RULE-2-2/DeadCode.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-2/DeadCode.testref b/c/misra/test/rules/RULE-2-2/DeadCode.testref deleted file mode 100644 index f084f30aaa..0000000000 --- a/c/misra/test/rules/RULE-2-2/DeadCode.testref +++ /dev/null @@ -1 +0,0 @@ -c/common/test/rules/deadcode/DeadCode.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-2/test.c b/c/misra/test/rules/RULE-2-2/test.c new file mode 100644 index 0000000000..148af8dc9e --- /dev/null +++ b/c/misra/test/rules/RULE-2-2/test.c @@ -0,0 +1,30 @@ +int may_have_side_effects(); +int no_side_effects(int x) { return 1 + 2; } +int no_side_effects_nondeterministic(); + +int test_dead_code(int x) { + int live1 = may_have_side_effects(), + live2 = may_have_side_effects(); // COMPLIANT + int live3 = 0, + live4 = may_have_side_effects(); // COMPLIANT + int live5 = 0, live6 = 0; // COMPLIANT + live5 = 1; // COMPLIANT + live6 = 2; // COMPLIANT + + int dead1 = 0, dead2 = 0; // COMPLIANT - init not considered by this rule + dead1 = 1; // NON_COMPLIANT - useless assignment + dead2 = 1; // NON_COMPLIANT - useless assignment + + may_have_side_effects(); // COMPLIANT + 1 + 2; // NON_COMPLIANT + + no_side_effects(x); // NON_COMPLIANT + + (int)may_have_side_effects(); // NON_COMPLIANT + (int)no_side_effects(x); // NON_COMPLIANT + (void)no_side_effects(x); // COMPLIANT + (may_have_side_effects()); // COMPLIANT + (no_side_effects(x)); // NON_COMPLIANT + + return live5 + live6; // COMPLIANT +} \ No newline at end of file diff --git a/rule_packages/c/DeadCode.json b/rule_packages/c/DeadCode.json index 1de7625225..21c8a94ac8 100644 --- a/rule_packages/c/DeadCode.json +++ b/rule_packages/c/DeadCode.json @@ -39,21 +39,7 @@ "tags": [ "readability", "maintainability" - ], - "implementation_scope": { - "description": "This query identifies dead statements in the program of the following kinds:", - "items": [ - "Declarations of a non-static stack variable whose initializing expression is pure (i.e. has no side-effects) and that is never subsequently accessed in live code.", - "Blocks that contain only dead statements.", - "Do loops whose condition is pure, and whose body contains only dead statements.", - "If statements whose condition is pure, and whose then and else clauses (where they exist) only contain dead statements.", - "Label statements to which the code never jumps.", - "While loops whose condition is pure, and whose body contains only dead statements.", - "Expression statements whose expressions are pure.", - "Writes to a non-static stack variable that is never subsequently read in live code." - ] - }, - "shared_implementation_short_name": "DeadCode" + ] } ], "title": "There shall be no dead code" From 85b161a2d29826cc292cabb57e84487af342375b Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 26 Sep 2024 09:13:20 +0100 Subject: [PATCH 07/12] RULE-2-2: Exclude cases nested in macro expansions --- c/misra/src/rules/RULE-2-2/DeadCode.ql | 4 ++++ c/misra/test/rules/RULE-2-2/DeadCode.expected | 2 ++ c/misra/test/rules/RULE-2-2/test.c | 12 ++++++++++++ .../cpp/alertreporting/HoldsForAllInstances.qll | 15 ++++++++++----- 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/c/misra/src/rules/RULE-2-2/DeadCode.ql b/c/misra/src/rules/RULE-2-2/DeadCode.ql index 79f69e760d..f90a11eb70 100644 --- a/c/misra/src/rules/RULE-2-2/DeadCode.ql +++ b/c/misra/src/rules/RULE-2-2/DeadCode.ql @@ -14,6 +14,7 @@ import cpp import codingstandards.c.misra +import codingstandards.cpp.alertreporting.HoldsForAllInstances import codingstandards.cpp.deadcode.UselessAssignments /** @@ -39,6 +40,9 @@ class DeadOperation extends Expr { string description; DeadOperation() { + // Exclude cases nested within macro expansions, because the code may be "live" in other + // expansions + isNotWithinMacroExpansion(this) and exists(ExprStmtExpr e | if exists(getExplicitCast(e)) then diff --git a/c/misra/test/rules/RULE-2-2/DeadCode.expected b/c/misra/test/rules/RULE-2-2/DeadCode.expected index 6cb0fb0c60..1f4ff5f4a8 100644 --- a/c/misra/test/rules/RULE-2-2/DeadCode.expected +++ b/c/misra/test/rules/RULE-2-2/DeadCode.expected @@ -5,3 +5,5 @@ | test.c:23:3:23:30 | (int)... | Cast operation is unused. | | test.c:24:3:24:25 | (int)... | Cast operation is unused. | | test.c:27:4:27:18 | call to no_side_effects | Result of operation is unused and has no side effects. | +| test.c:37:3:37:27 | call to no_side_effects | Result of operation is unused and has no side effects. | +| test.c:38:7:38:31 | call to no_side_effects | Result of operation is unused and has no side effects. | diff --git a/c/misra/test/rules/RULE-2-2/test.c b/c/misra/test/rules/RULE-2-2/test.c index 148af8dc9e..f8248c52d2 100644 --- a/c/misra/test/rules/RULE-2-2/test.c +++ b/c/misra/test/rules/RULE-2-2/test.c @@ -26,5 +26,17 @@ int test_dead_code(int x) { (may_have_side_effects()); // COMPLIANT (no_side_effects(x)); // NON_COMPLIANT +#define FULL_STMT_NO_SIDE_EFFECTS no_side_effects(1); +#define PART_STMT_NO_SIDE_EFFECTS no_side_effects(1) +#define BLOCK_SOME_SIDE_EFFECTS \ + { \ + may_have_side_effects(); \ + no_side_effects(1); \ + } + + FULL_STMT_NO_SIDE_EFFECTS // NON_COMPLIANT + PART_STMT_NO_SIDE_EFFECTS; // NON_COMPLIANT + BLOCK_SOME_SIDE_EFFECTS; // COMPLIANT + return live5 + live6; // COMPLIANT } \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll index fcf307c975..aa2abd9e88 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll @@ -20,14 +20,19 @@ import cpp /** - * Holds if the `Stmt` `s` is not within a macro expansion, i.e. generated by a macro, but not the - * outermost `Stmt` generated by the macro. + * Holds if the `Element` `e` is not within a macro expansion, i.e. generated by a macro, but not + * the outermost `Stmt` or `Expr` generated by the macro. */ -predicate isNotWithinMacroExpansion(Stmt s) { - not s.isInMacroExpansion() +predicate isNotWithinMacroExpansion(Element e) { + not e.isInMacroExpansion() or exists(MacroInvocation mi | - mi.getStmt() = s and + mi.getStmt() = e + or + mi.getExpr() = e + or + mi.getStmt().(ExprStmt).getExpr() = e + | not exists(mi.getParentInvocation()) ) } From 0639b1992c239b17ae292ae7e9c5c943a06c7d2f Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 26 Sep 2024 22:53:39 +0100 Subject: [PATCH 08/12] Rule 2.2: Ignore results which are only dead in some compilations Use the HoldsForAllInstances module to eliminate cases where a line of code is compiled into multiple targets with different dead code behaviour. --- c/misra/src/rules/RULE-2-2/DeadCode.ql | 10 +-- .../alertreporting/HoldsForAllInstances.qll | 69 ++++++++++--------- .../cpp/rules/deadcode/DeadCode.qll | 8 +-- 3 files changed, 47 insertions(+), 40 deletions(-) diff --git a/c/misra/src/rules/RULE-2-2/DeadCode.ql b/c/misra/src/rules/RULE-2-2/DeadCode.ql index f90a11eb70..9c2671e1f4 100644 --- a/c/misra/src/rules/RULE-2-2/DeadCode.ql +++ b/c/misra/src/rules/RULE-2-2/DeadCode.ql @@ -36,10 +36,10 @@ class ExprStmtExpr extends Expr { * An "operation" as defined by MISRA C Rule 2.2 that is dead, i.e. it's removal has no effect on * the behaviour of the program. */ -class DeadOperation extends Expr { +class DeadOperationInstance extends Expr { string description; - DeadOperation() { + DeadOperationInstance() { // Exclude cases nested within macro expansions, because the code may be "live" in other // expansions isNotWithinMacroExpansion(this) and @@ -74,6 +74,8 @@ class DeadOperation extends Expr { string getDescription() { result = description } } +class DeadOperation = HoldsForAllInstances::LogicalResultElement; + from DeadOperation deadOperation -where not isExcluded(deadOperation, DeadCodePackage::deadCodeQuery()) -select deadOperation, deadOperation.getDescription() + "." +where not isExcluded(deadOperation.getAnElementInstance(), DeadCodePackage::deadCodeQuery()) +select deadOperation, deadOperation.getAnElementInstance().getDescription() + "." diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll index aa2abd9e88..1ea8787c22 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll @@ -2,26 +2,26 @@ * A module for considering whether a result occurs in all instances (e.g. copies) of the code at a * given location. * - * Multiple instances of a statement at the same location can occur for two main reasons: + * Multiple instances of an element at the same location can occur for two main reasons: * 1. Instantiations of a template * 2. Re-compilation of a file under a different context * This module helps ensure that a particular condition holds for all copies of a particular logical - * statement. For example, this can be used to determine whether a line of code is dead in all copies + * element. For example, this can be used to determine whether a line of code is dead in all copies * of a piece of code. * - * This module is parameterized by a set of _candidate_ statements in the program. For each candidate - * statement, we determine whether all other statements that occur at the same location in the - * program are also part of the same set, ignoring any results generated by macros. + * This module is parameterized by a set of _candidate_ elements in the program. For each candidate + * element, we determine whether all other elements in the same element set that occur at the same + * location in the program are also part of the same set, ignoring any results generated by macros. * - * We do so by reporting a new type of result, `LogicalResultStmt`, which represents a logical result - * where all instances of a statement at a given location are considered to be part of the same set. + * We do so by reporting a new type of result, `LogicalResultElement`, which represents a logical result + * where all instances of a element at a given location are considered to be part of the same set. */ import cpp /** * Holds if the `Element` `e` is not within a macro expansion, i.e. generated by a macro, but not - * the outermost `Stmt` or `Expr` generated by the macro. + * the outermost `Element` or `Expr` generated by the macro. */ predicate isNotWithinMacroExpansion(Element e) { not e.isInMacroExpansion() @@ -37,32 +37,35 @@ predicate isNotWithinMacroExpansion(Element e) { ) } -/** A candidate set of types. */ -signature class CandidateStmtSig extends Stmt; +/** A candidate set of elements. */ +signature class CandidateElementSig extends Element; + +/** The super set of relevant elements. */ +signature class ElementSetSig extends Element; /** * A module for considering whether a result occurs in all instances (e.g. copies) of the code at a * given location. */ -module HoldsForAllInstances { +module HoldsForAllInstances { private predicate hasLocation( - Stmt s, string filepath, int startline, int startcolumn, int endline, int endcolumn + ElementSet s, string filepath, int startline, int startcolumn, int endline, int endcolumn ) { s.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) } - final private class MyStmt = Stmt; + final private class MyElement = ElementSet; /** - * A `Stmt` that appears at the same location as a candidate statement. + * A `Element` that appears at the same location as a candidate element. */ - private class RelevantStmt extends MyStmt { - CandidateStmt s; + private class RelevantElement extends MyElement { + CandidateElement e; - RelevantStmt() { + RelevantElement() { exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | hasLocation(this, filepath, startline, startcolumn, endline, endcolumn) and - hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) + hasLocation(e, filepath, startline, startcolumn, endline, endcolumn) ) and // Not within a macro expansion, as we cannot match up instances by location in that // case @@ -71,41 +74,43 @@ module HoldsForAllInstances { not this instanceof Handler } - CandidateStmt getCandidateStmt() { result = s } + CandidateElement getCandidateElement() { result = e } } - newtype TResultStmts = - TLogicalResultStmt(string filepath, int startline, int startcolumn, int endline, int endcolumn) { - exists(CandidateStmt s | + newtype TResultElements = + TLogicalResultElement( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + exists(CandidateElement s | // Only consider candidates where we can match up the location isNotWithinMacroExpansion(s) and hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) and - // All relevant statements that occur at the same location are candidates - forex(RelevantStmt relevantStmt | s = relevantStmt.getCandidateStmt() | - relevantStmt instanceof CandidateStmt + // All relevant elements that occur at the same location are candidates + forex(RelevantElement relevantElement | s = relevantElement.getCandidateElement() | + relevantElement instanceof CandidateElement ) ) } /** - * A logical result statement, representing all instances of a statement that occur at the same + * A logical result element, representing all instances of a element that occur at the same * location. */ - class LogicalResultStmt extends TLogicalResultStmt { + class LogicalResultElement extends TLogicalResultElement { predicate hasLocationInfo( string filepath, int startline, int startcolumn, int endline, int endcolumn ) { - this = TLogicalResultStmt(filepath, startline, startcolumn, endline, endcolumn) + this = TLogicalResultElement(filepath, startline, startcolumn, endline, endcolumn) } - /** Gets an instance of this logical result statement. */ - CandidateStmt getAStmtInstance() { + /** Gets an instance of this logical result element. */ + CandidateElement getAnElementInstance() { exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | - this = TLogicalResultStmt(filepath, startline, startcolumn, endline, endcolumn) and + this = TLogicalResultElement(filepath, startline, startcolumn, endline, endcolumn) and hasLocation(result, filepath, startline, startcolumn, endline, endcolumn) ) } - string toString() { result = getAStmtInstance().toString() } + string toString() { result = getAnElementInstance().toString() } } } diff --git a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll index 667c9020a9..5023b8ae14 100644 --- a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll +++ b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll @@ -122,16 +122,16 @@ class DeadStmtInstance extends Stmt { } } -class DeadStmt = HoldsForAllInstances::LogicalResultStmt; +class DeadStmt = HoldsForAllInstances::LogicalResultElement; query predicate problems(DeadStmt s, string message) { - not isExcluded(s.getAStmtInstance(), getQuery()) and + not isExcluded(s.getAnElementInstance(), getQuery()) and message = "This statement is dead code." and // Report only the highest level dead statement, to avoid over reporting not exists(DeadStmt parent | // All instances must share a dead statement parent for us to report the parent instead - forall(Stmt instance | instance = s.getAStmtInstance() | - parent.getAStmtInstance() = instance.getParentStmt() + forall(Stmt instance | instance = s.getAnElementInstance() | + parent.getAnElementInstance() = instance.getParentStmt() ) ) } From 35d2f560d55e8737450d1f05a53766b9083dcc45 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 26 Sep 2024 23:08:12 +0100 Subject: [PATCH 09/12] Rule 2.2: Report dead function, if the op is a call --- c/misra/src/rules/RULE-2-2/DeadCode.ql | 21 ++++++++++++++++--- c/misra/test/rules/RULE-2-2/DeadCode.expected | 18 ++++++++-------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/c/misra/src/rules/RULE-2-2/DeadCode.ql b/c/misra/src/rules/RULE-2-2/DeadCode.ql index 9c2671e1f4..8d7ccce273 100644 --- a/c/misra/src/rules/RULE-2-2/DeadCode.ql +++ b/c/misra/src/rules/RULE-2-2/DeadCode.ql @@ -76,6 +76,21 @@ class DeadOperationInstance extends Expr { class DeadOperation = HoldsForAllInstances::LogicalResultElement; -from DeadOperation deadOperation -where not isExcluded(deadOperation.getAnElementInstance(), DeadCodePackage::deadCodeQuery()) -select deadOperation, deadOperation.getAnElementInstance().getDescription() + "." +from + DeadOperation deadOperation, DeadOperationInstance instance, string message, Element explainer, + string explainerDescription +where + not isExcluded(instance, DeadCodePackage::deadCodeQuery()) and + instance = deadOperation.getAnElementInstance() and + if instance instanceof FunctionCall + then + message = instance.getDescription() + " from call to function $@" and + explainer = instance.(FunctionCall).getTarget() and + explainerDescription = explainer.(Function).getName() + else ( + message = instance.getDescription() and + // Ignore the explainer + explainer = instance and + explainerDescription = "" + ) +select deadOperation, message + ".", explainer, explainerDescription diff --git a/c/misra/test/rules/RULE-2-2/DeadCode.expected b/c/misra/test/rules/RULE-2-2/DeadCode.expected index 1f4ff5f4a8..e25a5a97ef 100644 --- a/c/misra/test/rules/RULE-2-2/DeadCode.expected +++ b/c/misra/test/rules/RULE-2-2/DeadCode.expected @@ -1,9 +1,9 @@ -| test.c:15:3:15:11 | ... = ... | Assignment to dead1 is unused and has no side effects. | -| test.c:16:3:16:11 | ... = ... | Assignment to dead2 is unused and has no side effects. | -| test.c:19:3:19:7 | ... + ... | Result of operation is unused and has no side effects. | -| test.c:21:3:21:17 | call to no_side_effects | Result of operation is unused and has no side effects. | -| test.c:23:3:23:30 | (int)... | Cast operation is unused. | -| test.c:24:3:24:25 | (int)... | Cast operation is unused. | -| test.c:27:4:27:18 | call to no_side_effects | Result of operation is unused and has no side effects. | -| test.c:37:3:37:27 | call to no_side_effects | Result of operation is unused and has no side effects. | -| test.c:38:7:38:31 | call to no_side_effects | Result of operation is unused and has no side effects. | +| test.c:15:3:15:11 | ... = ... | Assignment to dead1 is unused and has no side effects. | test.c:15:3:15:11 | ... = ... | | +| test.c:16:3:16:11 | ... = ... | Assignment to dead2 is unused and has no side effects. | test.c:16:3:16:11 | ... = ... | | +| test.c:19:3:19:7 | ... + ... | Result of operation is unused and has no side effects. | test.c:19:3:19:7 | ... + ... | | +| test.c:21:3:21:17 | call to no_side_effects | Result of operation is unused and has no side effects from call to function $@. | test.c:2:5:2:19 | no_side_effects | no_side_effects | +| test.c:23:3:23:30 | (int)... | Cast operation is unused. | test.c:23:3:23:30 | (int)... | | +| test.c:24:3:24:25 | (int)... | Cast operation is unused. | test.c:24:3:24:25 | (int)... | | +| test.c:27:4:27:18 | call to no_side_effects | Result of operation is unused and has no side effects from call to function $@. | test.c:2:5:2:19 | no_side_effects | no_side_effects | +| test.c:37:3:37:27 | call to no_side_effects | Result of operation is unused and has no side effects from call to function $@. | test.c:2:5:2:19 | no_side_effects | no_side_effects | +| test.c:38:7:38:31 | call to no_side_effects | Result of operation is unused and has no side effects from call to function $@. | test.c:2:5:2:19 | no_side_effects | no_side_effects | From 785974b728b02f42b9a344ff1dd04c56652fd966 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 26 Sep 2024 23:12:33 +0100 Subject: [PATCH 10/12] DeadCode: Add change note --- change_notes/2024-09-25-dead-code-improvements.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 change_notes/2024-09-25-dead-code-improvements.md diff --git a/change_notes/2024-09-25-dead-code-improvements.md b/change_notes/2024-09-25-dead-code-improvements.md new file mode 100644 index 0000000000..9cd8d95ff5 --- /dev/null +++ b/change_notes/2024-09-25-dead-code-improvements.md @@ -0,0 +1,5 @@ + - `M0-1-9` - `DeadCode.ql` + - Remove false positives for statements where the enclosing function is compiled multiple times, either as part of different targets or a different template instantiations. Previously we would see false positives where a statement was dead in one instance of the code, but not other instances. We now only consider a statement dead if it is dead in all instances of that code. +- `RULE-2-2` - `DeadCode.ql`: + - Query has been rewritten to report only _operations_ that are considered dead, not statements. This should reduce false positives. + - Remove false positives for operations where the enclosing function is compiled multiple times, either as part of different targets or a different template instantiations. Previously we would see false positives where a operation was dead in one instance of the code, but not other instances. We now only consider a operation dead if it is dead in all instances of that code. \ No newline at end of file From 2a17ba73405d5a76d126173d597490a4f20df2e9 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 15 Oct 2024 11:40:02 +0100 Subject: [PATCH 11/12] Address review comments * Rename HoldsForAllInstances to HoldsForAllCopies * Improve documentation --- c/misra/src/rules/RULE-2-2/DeadCode.ql | 4 ++-- ...AllInstances.qll => HoldsForAllCopies.qll} | 23 +++++++++++-------- .../cpp/rules/deadcode/DeadCode.qll | 4 ++-- 3 files changed, 17 insertions(+), 14 deletions(-) rename cpp/common/src/codingstandards/cpp/alertreporting/{HoldsForAllInstances.qll => HoldsForAllCopies.qll} (82%) diff --git a/c/misra/src/rules/RULE-2-2/DeadCode.ql b/c/misra/src/rules/RULE-2-2/DeadCode.ql index 03a6e7d36a..97c3808607 100644 --- a/c/misra/src/rules/RULE-2-2/DeadCode.ql +++ b/c/misra/src/rules/RULE-2-2/DeadCode.ql @@ -15,7 +15,7 @@ import cpp import codingstandards.c.misra -import codingstandards.cpp.alertreporting.HoldsForAllInstances +import codingstandards.cpp.alertreporting.HoldsForAllCopies import codingstandards.cpp.deadcode.UselessAssignments /** @@ -75,7 +75,7 @@ class DeadOperationInstance extends Expr { string getDescription() { result = description } } -class DeadOperation = HoldsForAllInstances::LogicalResultElement; +class DeadOperation = HoldsForAllCopies::LogicalResultElement; from DeadOperation deadOperation, DeadOperationInstance instance, string message, Element explainer, diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll similarity index 82% rename from cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll rename to cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll index 1ea8787c22..634c1bf610 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllInstances.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll @@ -1,8 +1,7 @@ /** - * A module for considering whether a result occurs in all instances (e.g. copies) of the code at a - * given location. + * A module for considering whether a result occurs in all copies of the code at a given location. * - * Multiple instances of an element at the same location can occur for two main reasons: + * Multiple copies of an element at the same location can occur for two main reasons: * 1. Instantiations of a template * 2. Re-compilation of a file under a different context * This module helps ensure that a particular condition holds for all copies of a particular logical @@ -37,17 +36,21 @@ predicate isNotWithinMacroExpansion(Element e) { ) } -/** A candidate set of elements. */ +/** + * A type representing a set of Element's in the program that satisfy some condition. + * + * `HoldsForAllCopies::LogicalResultElement` will represent an element in this set + * iff all copies of that element satisfy the condition. + */ signature class CandidateElementSig extends Element; /** The super set of relevant elements. */ signature class ElementSetSig extends Element; /** - * A module for considering whether a result occurs in all instances (e.g. copies) of the code at a - * given location. + * A module for considering whether a result occurs in all copies of the code at a given location. */ -module HoldsForAllInstances { +module HoldsForAllCopies { private predicate hasLocation( ElementSet s, string filepath, int startline, int startcolumn, int endline, int endcolumn ) { @@ -93,8 +96,8 @@ module HoldsForAllInstances::LogicalResultElement; +class DeadStmt = HoldsForAllCopies::LogicalResultElement; query predicate problems(DeadStmt s, string message) { not isExcluded(s.getAnElementInstance(), getQuery()) and From 43bf6f87ded8e77ced5838009fdcee705d67f8e8 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 15 Oct 2024 17:49:46 +0100 Subject: [PATCH 12/12] Fix DeadCode.json syntax error introduced on merge --- rule_packages/c/DeadCode.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rule_packages/c/DeadCode.json b/rule_packages/c/DeadCode.json index fbcdfe2976..d8e80d14d1 100644 --- a/rule_packages/c/DeadCode.json +++ b/rule_packages/c/DeadCode.json @@ -39,7 +39,7 @@ "short_name": "DeadCode", "tags": [ "readability", - "maintainability" + "maintainability", "external/misra/c/2012/third-edition-first-revision" ] }