Skip to content

Commit dcab086

Browse files
lcarteylcartey
authored
Merge pull request #717 from github/lcartey/dead-code-improvements
`DeadCode`: Eliminate out-of-scope results, handle templates and multiple compilation
2 parents 3f73b26 + 43bf6f8 commit dcab086

File tree

11 files changed

+335
-34
lines changed

11 files changed

+335
-34
lines changed

‎c/misra/src/rules/RULE-2-2/DeadCode.ql

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,83 @@
1515

1616
import cpp
1717
import codingstandards.c.misra
18-
import codingstandards.cpp.rules.deadcode.DeadCode
18+
import codingstandards.cpp.alertreporting.HoldsForAllCopies
19+
import codingstandards.cpp.deadcode.UselessAssignments
1920

20-
class MisraCDeadCodeQuery extends DeadCodeSharedQuery {
21-
MisraCDeadCodeQuery() { this = DeadCodePackage::deadCodeQuery() }
21+
/**
22+
* Gets an explicit cast from `e` if one exists.
23+
*/
24+
Cast getExplicitCast(Expr e) {
25+
exists(Conversion c | c = e.getExplicitlyConverted() |
26+
result = c
27+
or
28+
result = c.(ParenthesisExpr).getExpr()
29+
)
30+
}
31+
32+
class ExprStmtExpr extends Expr {
33+
ExprStmtExpr() { exists(ExprStmt es | es.getExpr() = this) }
34+
}
35+
36+
/**
37+
* An "operation" as defined by MISRA C Rule 2.2 that is dead, i.e. it's removal has no effect on
38+
* the behaviour of the program.
39+
*/
40+
class DeadOperationInstance extends Expr {
41+
string description;
42+
43+
DeadOperationInstance() {
44+
// Exclude cases nested within macro expansions, because the code may be "live" in other
45+
// expansions
46+
isNotWithinMacroExpansion(this) and
47+
exists(ExprStmtExpr e |
48+
if exists(getExplicitCast(e))
49+
then
50+
this = getExplicitCast(e) and
51+
// void conversions are permitted
52+
not getExplicitCast(e) instanceof VoidConversion and
53+
description = "Cast operation is unused"
54+
else (
55+
this = e and
56+
(
57+
if e instanceof Assignment
58+
then
59+
exists(SsaDefinition sd, LocalScopeVariable v |
60+
e = sd.getDefinition() and
61+
sd.getDefiningValue(v).isPure() and
62+
// The definition is useless
63+
isUselessSsaDefinition(sd, v) and
64+
description = "Assignment to " + v.getName() + " is unused and has no side effects"
65+
)
66+
else (
67+
e.isPure() and
68+
description = "Result of operation is unused and has no side effects"
69+
)
70+
)
71+
)
72+
)
73+
}
74+
75+
string getDescription() { result = description }
2276
}
77+
78+
class DeadOperation = HoldsForAllCopies<DeadOperationInstance, Expr>::LogicalResultElement;
79+
80+
from
81+
DeadOperation deadOperation, DeadOperationInstance instance, string message, Element explainer,
82+
string explainerDescription
83+
where
84+
not isExcluded(instance, DeadCodePackage::deadCodeQuery()) and
85+
instance = deadOperation.getAnElementInstance() and
86+
if instance instanceof FunctionCall
87+
then
88+
message = instance.getDescription() + " from call to function $@" and
89+
explainer = instance.(FunctionCall).getTarget() and
90+
explainerDescription = explainer.(Function).getName()
91+
else (
92+
message = instance.getDescription() and
93+
// Ignore the explainer
94+
explainer = instance and
95+
explainerDescription = ""
96+
)
97+
select deadOperation, message + ".", explainer, explainerDescription

‎c/misra/test/rules/RULE-2-2/DeadCode.expected

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
| test.c:15:3:15:11 | ... = ... | Assignment to dead1 is unused and has no side effects. | test.c:15:3:15:11 | ... = ... | |
2+
| test.c:16:3:16:11 | ... = ... | Assignment to dead2 is unused and has no side effects. | test.c:16:3:16:11 | ... = ... | |
3+
| test.c:19:3:19:7 | ... + ... | Result of operation is unused and has no side effects. | test.c:19:3:19:7 | ... + ... | |
4+
| 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 |
5+
| test.c:23:3:23:30 | (int)... | Cast operation is unused. | test.c:23:3:23:30 | (int)... | |
6+
| test.c:24:3:24:25 | (int)... | Cast operation is unused. | test.c:24:3:24:25 | (int)... | |
7+
| 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 |
8+
| 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 |
9+
| 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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
rules/RULE-2-2/DeadCode.ql

‎c/misra/test/rules/RULE-2-2/DeadCode.testref

Lines changed: 0 additions & 1 deletion
This file was deleted.

‎c/misra/test/rules/RULE-2-2/test.c

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
int may_have_side_effects();
2+
int no_side_effects(int x) { return 1 + 2; }
3+
int no_side_effects_nondeterministic();
4+
5+
int test_dead_code(int x) {
6+
int live1 = may_have_side_effects(),
7+
live2 = may_have_side_effects(); // COMPLIANT
8+
int live3 = 0,
9+
live4 = may_have_side_effects(); // COMPLIANT
10+
int live5 = 0, live6 = 0; // COMPLIANT
11+
live5 = 1; // COMPLIANT
12+
live6 = 2; // COMPLIANT
13+
14+
int dead1 = 0, dead2 = 0; // COMPLIANT - init not considered by this rule
15+
dead1 = 1; // NON_COMPLIANT - useless assignment
16+
dead2 = 1; // NON_COMPLIANT - useless assignment
17+
18+
may_have_side_effects(); // COMPLIANT
19+
1 + 2; // NON_COMPLIANT
20+
21+
no_side_effects(x); // NON_COMPLIANT
22+
23+
(int)may_have_side_effects(); // NON_COMPLIANT
24+
(int)no_side_effects(x); // NON_COMPLIANT
25+
(void)no_side_effects(x); // COMPLIANT
26+
(may_have_side_effects()); // COMPLIANT
27+
(no_side_effects(x)); // NON_COMPLIANT
28+
29+
#define FULL_STMT_NO_SIDE_EFFECTS no_side_effects(1);
30+
#define PART_STMT_NO_SIDE_EFFECTS no_side_effects(1)
31+
#define BLOCK_SOME_SIDE_EFFECTS \
32+
{ \
33+
may_have_side_effects(); \
34+
no_side_effects(1); \
35+
}
36+
37+
FULL_STMT_NO_SIDE_EFFECTS // NON_COMPLIANT
38+
PART_STMT_NO_SIDE_EFFECTS; // NON_COMPLIANT
39+
BLOCK_SOME_SIDE_EFFECTS; // COMPLIANT
40+
41+
return live5 + live6; // COMPLIANT
42+
}

‎change_notes/2024-09-25-dead-code-improvements.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
- `M0-1-9` - `DeadCode.ql`
2+
- 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.
3+
- `RULE-2-2` - `DeadCode.ql`:
4+
- Query has been rewritten to report only _operations_ that are considered dead, not statements. This should reduce false positives.
5+
- 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

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* A module for considering whether a result occurs in all copies of the code at a given location.
3+
*
4+
* Multiple copies of an element at the same location can occur for two main reasons:
5+
* 1. Instantiations of a template
6+
* 2. Re-compilation of a file under a different context
7+
* This module helps ensure that a particular condition holds for all copies of a particular logical
8+
* element. For example, this can be used to determine whether a line of code is dead in all copies
9+
* of a piece of code.
10+
*
11+
* This module is parameterized by a set of _candidate_ elements in the program. For each candidate
12+
* element, we determine whether all other elements in the same element set that occur at the same
13+
* location in the program are also part of the same set, ignoring any results generated by macros.
14+
*
15+
* We do so by reporting a new type of result, `LogicalResultElement`, which represents a logical result
16+
* where all instances of a element at a given location are considered to be part of the same set.
17+
*/
18+
19+
import cpp
20+
21+
/**
22+
* Holds if the `Element` `e` is not within a macro expansion, i.e. generated by a macro, but not
23+
* the outermost `Element` or `Expr` generated by the macro.
24+
*/
25+
predicate isNotWithinMacroExpansion(Element e) {
26+
not e.isInMacroExpansion()
27+
or
28+
exists(MacroInvocation mi |
29+
mi.getStmt() = e
30+
or
31+
mi.getExpr() = e
32+
or
33+
mi.getStmt().(ExprStmt).getExpr() = e
34+
|
35+
not exists(mi.getParentInvocation())
36+
)
37+
}
38+
39+
/**
40+
* A type representing a set of Element's in the program that satisfy some condition.
41+
*
42+
* `HoldsForAllCopies<T>::LogicalResultElement` will represent an element in this set
43+
* iff all copies of that element satisfy the condition.
44+
*/
45+
signature class CandidateElementSig extends Element;
46+
47+
/** The super set of relevant elements. */
48+
signature class ElementSetSig extends Element;
49+
50+
/**
51+
* A module for considering whether a result occurs in all copies of the code at a given location.
52+
*/
53+
module HoldsForAllCopies<CandidateElementSig CandidateElement, ElementSetSig ElementSet> {
54+
private predicate hasLocation(
55+
ElementSet s, string filepath, int startline, int startcolumn, int endline, int endcolumn
56+
) {
57+
s.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
58+
}
59+
60+
final private class MyElement = ElementSet;
61+
62+
/**
63+
* A `Element` that appears at the same location as a candidate element.
64+
*/
65+
private class RelevantElement extends MyElement {
66+
CandidateElement e;
67+
68+
RelevantElement() {
69+
exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
70+
hasLocation(this, filepath, startline, startcolumn, endline, endcolumn) and
71+
hasLocation(e, filepath, startline, startcolumn, endline, endcolumn)
72+
) and
73+
// Not within a macro expansion, as we cannot match up instances by location in that
74+
// case
75+
isNotWithinMacroExpansion(this) and
76+
// Ignore catch handlers, as they occur at the same location as the catch block
77+
not this instanceof Handler
78+
}
79+
80+
CandidateElement getCandidateElement() { result = e }
81+
}
82+
83+
newtype TResultElements =
84+
TLogicalResultElement(
85+
string filepath, int startline, int startcolumn, int endline, int endcolumn
86+
) {
87+
exists(CandidateElement s |
88+
// Only consider candidates where we can match up the location
89+
isNotWithinMacroExpansion(s) and
90+
hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) and
91+
// All relevant elements that occur at the same location are candidates
92+
forex(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
93+
relevantElement instanceof CandidateElement
94+
)
95+
)
96+
}
97+
98+
/**
99+
* A logical result element representing all copies of an element that occur at the same
100+
* location, iff they all belong to the `CandidateElement` set.
101+
*/
102+
class LogicalResultElement extends TLogicalResultElement {
103+
predicate hasLocationInfo(
104+
string filepath, int startline, int startcolumn, int endline, int endcolumn
105+
) {
106+
this = TLogicalResultElement(filepath, startline, startcolumn, endline, endcolumn)
107+
}
108+
109+
/** Gets a copy instance of this logical result element. */
110+
CandidateElement getAnElementInstance() {
111+
exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
112+
this = TLogicalResultElement(filepath, startline, startcolumn, endline, endcolumn) and
113+
hasLocation(result, filepath, startline, startcolumn, endline, endcolumn)
114+
)
115+
}
116+
117+
string toString() { result = getAnElementInstance().toString() }
118+
}
119+
}

‎cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
*/
1313

1414
import cpp
15+
import codingstandards.cpp.alertreporting.HoldsForAllCopies
1516
import codingstandards.cpp.Customizations
1617
import codingstandards.cpp.Exclusions
1718
import codingstandards.cpp.deadcode.UselessAssignments
@@ -31,10 +32,6 @@ predicate isDeadOrUnreachableStmt(Stmt s) {
3132
s.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock()
3233
}
3334

34-
/**
35-
* Holds if the `Stmt` `s` is dead, i.e. could be executed, but its removal would not meaningfully
36-
* affect the program.
37-
*/
3835
predicate isDeadStmt(Stmt s) {
3936
// A `DeclStmt` is dead code if:
4037
// - All the declarations are variable declarations
@@ -108,17 +105,33 @@ predicate isDeadStmt(Stmt s) {
108105
exists(TryStmt ts | s = ts and isDeadStmt(ts.getStmt()))
109106
}
110107

111-
query predicate problems(Stmt s, string message) {
112-
not isExcluded(s, getQuery()) and
108+
/**
109+
* Holds if the `Stmt` `s` is dead, i.e. could be executed, but its removal would not meaningfully
110+
* affect the program.
111+
*/
112+
class DeadStmtInstance extends Stmt {
113+
DeadStmtInstance() {
114+
isDeadStmt(this) and
115+
// Exclude compiler generated statements
116+
not this.isCompilerGenerated() and
117+
// Exclude code fully generated by macros, because the code may be "live" in other expansions
118+
isNotWithinMacroExpansion(this) and
119+
// MISRA defines dead code as an "_executed_ statement whose removal would not affect the program
120+
// output". We therefore exclude unreachable statements as they are, by definition, not executed.
121+
not this.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock()
122+
}
123+
}
124+
125+
class DeadStmt = HoldsForAllCopies<DeadStmtInstance, Stmt>::LogicalResultElement;
126+
127+
query predicate problems(DeadStmt s, string message) {
128+
not isExcluded(s.getAnElementInstance(), getQuery()) and
113129
message = "This statement is dead code." and
114-
isDeadStmt(s) and
115130
// Report only the highest level dead statement, to avoid over reporting
116-
not isDeadStmt(s.getParentStmt()) and
117-
// MISRA defines dead code as an "_executed_ statement whose removal would not affect the program
118-
// output". We therefore exclude unreachable statements as they are, by definition, not executed.
119-
not s.getBasicBlock() = any(UnreachableBasicBlock ubb).getABasicBlock() and
120-
// Exclude code fully generated by macros, because the code may be "live" in other expansions
121-
not s.isInMacroExpansion() and
122-
// Exclude compiler generated statements
123-
not s.isCompilerGenerated()
131+
not exists(DeadStmt parent |
132+
// All instances must share a dead statement parent for us to report the parent instead
133+
forall(Stmt instance | instance = s.getAnElementInstance() |
134+
parent.getAnElementInstance() = instance.getParentStmt()
135+
)
136+
)
124137
}

‎cpp/common/test/rules/deadcode/DeadCode.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,7 @@
1515
| test.cpp:85:3:85:43 | declaration | This statement is dead code. |
1616
| test.cpp:87:3:87:30 | declaration | This statement is dead code. |
1717
| test.cpp:90:3:90:50 | declaration | This statement is dead code. |
18+
| test.cpp:116:3:116:21 | ExprStmt | This statement is dead code. |
19+
| test.cpp:117:3:117:27 | ExprStmt | This statement is dead code. |
20+
| test.cpp:118:7:118:32 | ExprStmt | This statement is dead code. |
21+
| test.cpp:139:3:139:35 | ExprStmt | This statement is dead code. |

‎cpp/common/test/rules/deadcode/test.cpp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,3 +91,51 @@ int test_dead_code(int x) {
9191

9292
return live5 + live6 + constexpr_used_array[1]; // COMPLIANT
9393
}
94+
95+
class Foo {
96+
public:
97+
void bar() { may_have_side_effects(); }
98+
};
99+
100+
class Baz {
101+
public:
102+
void bar() {} // No side effects
103+
};
104+
105+
#define FULL_STMT_NO_SIDE_EFFECTS no_side_effects(1);
106+
#define PART_STMT_NO_SIDE_EFFECTS no_side_effects(1)
107+
#define BLOCK_SOME_SIDE_EFFECTS \
108+
{ \
109+
may_have_side_effects(); \
110+
no_side_effects(1); \
111+
}
112+
113+
template <typename T> void test_template() {
114+
T t;
115+
t.bar(); // COMPLIANT
116+
no_side_effects(1); // NON_COMPLIANT
117+
FULL_STMT_NO_SIDE_EFFECTS // NON_COMPLIANT
118+
PART_STMT_NO_SIDE_EFFECTS; // NON_COMPLIANT
119+
BLOCK_SOME_SIDE_EFFECTS; // COMPLIANT - cannot determine loc for
120+
// no_side_effects(1)
121+
}
122+
123+
template <typename T> void test_variant_side_effects() {
124+
T t;
125+
t.bar(); // COMPLIANT - not dead in at least one instance
126+
}
127+
128+
template <typename T> void test_unused_template() {
129+
T t;
130+
t.bar(); // COMPLIANT
131+
no_side_effects(
132+
1); // NON_COMPLIANT[FALSE_NEGATIVE] - unused templates are not extracted
133+
}
134+
135+
void test() {
136+
test_template<Foo>();
137+
test_template<Baz>();
138+
test_variant_side_effects<Foo>(); // COMPLIANT
139+
test_variant_side_effects<Baz>(); // NON_COMPLIANT - no effect in this
140+
// instantiation
141+
}

0 commit comments

Comments
 (0)