diff --git a/c/misra/src/rules/RULE-2-2/DeadCode.ql b/c/misra/src/rules/RULE-2-2/DeadCode.ql index 19ac69c2c1..97c3808607 100644 --- a/c/misra/src/rules/RULE-2-2/DeadCode.ql +++ b/c/misra/src/rules/RULE-2-2/DeadCode.ql @@ -15,8 +15,83 @@ import cpp import codingstandards.c.misra -import codingstandards.cpp.rules.deadcode.DeadCode +import codingstandards.cpp.alertreporting.HoldsForAllCopies +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 DeadOperationInstance extends Expr { + string description; + + DeadOperationInstance() { + // 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 + 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 } } + +class DeadOperation = HoldsForAllCopies::LogicalResultElement; + +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 new file mode 100644 index 0000000000..e25a5a97ef --- /dev/null +++ b/c/misra/test/rules/RULE-2-2/DeadCode.expected @@ -0,0 +1,9 @@ +| 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 | 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..f8248c52d2 --- /dev/null +++ b/c/misra/test/rules/RULE-2-2/test.c @@ -0,0 +1,42 @@ +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 + +#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/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 diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll new file mode 100644 index 0000000000..634c1bf610 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll @@ -0,0 +1,119 @@ +/** + * A module for considering whether a result occurs in all copies of the code at a given location. + * + * 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 + * 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_ 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, `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 `Element` or `Expr` generated by the macro. + */ +predicate isNotWithinMacroExpansion(Element e) { + not e.isInMacroExpansion() + or + exists(MacroInvocation mi | + mi.getStmt() = e + or + mi.getExpr() = e + or + mi.getStmt().(ExprStmt).getExpr() = e + | + not exists(mi.getParentInvocation()) + ) +} + +/** + * 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 copies of the code at a given location. + */ +module HoldsForAllCopies { + private predicate hasLocation( + ElementSet s, string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + s.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } + + final private class MyElement = ElementSet; + + /** + * A `Element` that appears at the same location as a candidate element. + */ + private class RelevantElement extends MyElement { + CandidateElement e; + + RelevantElement() { + exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | + hasLocation(this, filepath, startline, startcolumn, endline, endcolumn) and + hasLocation(e, 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 + } + + CandidateElement getCandidateElement() { result = e } + } + + 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 elements that occur at the same location are candidates + forex(RelevantElement relevantElement | s = relevantElement.getCandidateElement() | + relevantElement instanceof CandidateElement + ) + ) + } + + /** + * A logical result element representing all copies of an element that occur at the same + * location, iff they all belong to the `CandidateElement` set. + */ + class LogicalResultElement extends TLogicalResultElement { + predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + this = TLogicalResultElement(filepath, startline, startcolumn, endline, endcolumn) + } + + /** Gets a copy instance of this logical result element. */ + CandidateElement getAnElementInstance() { + exists(string filepath, int startline, int startcolumn, int endline, int endcolumn | + this = TLogicalResultElement(filepath, startline, startcolumn, endline, endcolumn) and + hasLocation(result, filepath, startline, startcolumn, endline, endcolumn) + ) + } + + 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 2b5be15e80..fb0bd772a2 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.HoldsForAllCopies 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 + 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() + } +} + +class DeadStmt = HoldsForAllCopies::LogicalResultElement; + +query predicate problems(DeadStmt s, string message) { + not isExcluded(s.getAnElementInstance(), 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.getAnElementInstance() | + parent.getAnElementInstance() = instance.getParentStmt() + ) + ) } diff --git a/cpp/common/test/rules/deadcode/DeadCode.expected b/cpp/common/test/rules/deadcode/DeadCode.expected index aec93e0914..1756231343 100644 --- a/cpp/common/test/rules/deadcode/DeadCode.expected +++ b/cpp/common/test/rules/deadcode/DeadCode.expected @@ -15,3 +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: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 d9c0cab277..d40667539d 100644 --- a/cpp/common/test/rules/deadcode/test.cpp +++ b/cpp/common/test/rules/deadcode/test.cpp @@ -91,3 +91,51 @@ 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 +}; + +#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 + 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() { + T t; + t.bar(); // COMPLIANT + no_side_effects( + 1); // NON_COMPLIANT[FALSE_NEGATIVE] - unused templates are not extracted +} + +void test() { + test_template(); + test_template(); + test_variant_side_effects(); // COMPLIANT + test_variant_side_effects(); // NON_COMPLIANT - no effect in this + // instantiation +} \ No newline at end of file diff --git a/rule_packages/c/DeadCode.json b/rule_packages/c/DeadCode.json index ea5ddad703..d8e80d14d1 100644 --- a/rule_packages/c/DeadCode.json +++ b/rule_packages/c/DeadCode.json @@ -41,21 +41,7 @@ "readability", "maintainability", "external/misra/c/2012/third-edition-first-revision" - ], - "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"