Merge pull request #717 from github/lcartey/dead-code-improvements

`DeadCode`: Eliminate out-of-scope results, handle templates and multiple compilation

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<DeadOperationInstance, Expr>::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

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 |

c/misra/test/rules/RULE-2-2/DeadCode.qlref

@@ -0,0 +1 @@
+rules/RULE-2-2/DeadCode.ql

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

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.

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<T>::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<CandidateElementSig CandidateElement, ElementSetSig ElementSet> {
+  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() }
+  }
+}

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<DeadStmtInstance, Stmt>::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()
+    )
+  )
 }

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

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 <typename T> 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 <typename T> void test_variant_side_effects() {
+  T t;
+  t.bar(); // COMPLIANT - not dead in at least one instance
+}
+
+template <typename T> 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<Foo>();
+  test_template<Baz>();
+  test_variant_side_effects<Foo>(); // COMPLIANT
+  test_variant_side_effects<Baz>(); // NON_COMPLIANT - no effect in this
+                                    // instantiation
+}