From c765f93757323ee5218c0670016ec4b76f3387e8 Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Thu, 12 Sep 2024 12:22:50 +0900 Subject: [PATCH 1/7] Fix(common/cpp): dead code alert on constexpr with array sizes. --- .../cpp/rules/deadcode/DeadCode.qll | 24 ++++++++++++++++++- .../test/rules/deadcode/DeadCode.expected | 2 ++ cpp/common/test/rules/deadcode/test.cpp | 5 ++++ 3 files changed, 30 insertions(+), 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 4a008dc15a..5f3b77e661 100644 --- a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll +++ b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll @@ -16,11 +16,31 @@ import codingstandards.cpp.Customizations import codingstandards.cpp.Exclusions import codingstandards.cpp.deadcode.UselessAssignments import codingstandards.cpp.deadcode.UnreachableCode +import codingstandards.cpp.deadcode.UnusedVariables abstract class DeadCodeSharedQuery extends Query { } Query getQuery() { result instanceof DeadCodeSharedQuery } +/** + * Returns integer value of a constexpr variable + */ +int getConstexprValue(Variable v) { + result = v.getInitializer().getExpr().getValue().toInt() and v.isConstexpr() +} + +/** + * Holds if `Variable` v is used for a local array size with value `n` + */ +bindingset[n] +predicate isUsedInLocalArraySize(Variable v, int n) { + // Cf. https://github.com/github/codeql-coding-standards/pull/660/files. + count(ArrayType at, LocalVariable arrayVariable | + arrayVariable.getType().resolveTypedefs() = at and + v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and + at.getArraySize() = n) > 0 +} + /** * Holds if the `Stmt` `s` is either dead or unreachable. */ @@ -39,6 +59,7 @@ predicate isDeadStmt(Stmt s) { // - All the declarations are variable declarations // - None of those variables are ever accessed in non-dead code // - The initializers for each of the variables are pure + // - It isn't constexpr and used to declare an array size exists(DeclStmt ds | ds = s and // Use forex so that we don't flag "fake" generated `DeclStmt`s (e.g. those generated by the @@ -50,7 +71,8 @@ predicate isDeadStmt(Stmt s) { not exists(VariableAccess va | va.getTarget() = v and not isDeadOrUnreachableStmt(va.getEnclosingStmt()) - ) + ) and + not isUsedInLocalArraySize(v, getConstexprValue(v)) ) ) ) diff --git a/cpp/common/test/rules/deadcode/DeadCode.expected b/cpp/common/test/rules/deadcode/DeadCode.expected index 6c111d8a93..d3252e9401 100644 --- a/cpp/common/test/rules/deadcode/DeadCode.expected +++ b/cpp/common/test/rules/deadcode/DeadCode.expected @@ -12,3 +12,5 @@ | test.cpp:72:3:73:3 | try { ... } | This statement is dead code. | | test.cpp:73:17:74:3 | { ... } | This statement is dead code. | | test.cpp:79:17:80:3 | { ... } | This statement is dead code. | +| test.cpp:85:3:85:44 | declaration | This statement is dead code. | +| test.cpp:87:3:87:30 | declaration | This statement is dead code. | diff --git a/cpp/common/test/rules/deadcode/test.cpp b/cpp/common/test/rules/deadcode/test.cpp index ba5c59b07c..982c021563 100644 --- a/cpp/common/test/rules/deadcode/test.cpp +++ b/cpp/common/test/rules/deadcode/test.cpp @@ -81,5 +81,10 @@ int test_dead_code(int x) { static_assert(1); // COMPLIANT + constexpr int constexpr_array_size{6}; // COMPLIANT + int unused_array[constexpr_array_size] {}; // NON_COMPLIANT + + constexpr int unused_int{2}; // NON_COMPLIANT + return live5 + live6; // COMPLIANT } \ No newline at end of file From 15801d9245b453c599f6445b0f52c54c2261e8dd Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Mon, 16 Sep 2024 20:56:00 +0900 Subject: [PATCH 2/7] Refactor to share a predicate between M0-1-3 and M0-1-9 reducing duplication. --- .../src/rules/M0-1-3/UnusedLocalVariable.ql | 6 +----- .../cpp/deadcode/UnusedVariables.qll | 16 ++++++++++++++ .../cpp/rules/deadcode/DeadCode.qll | 21 +------------------ 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql index 5956515e5b..0387208514 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql @@ -44,11 +44,7 @@ int getUseCountConservatively(Variable v) { count(StaticAssert s | s.getCondition().getAChild*().getValue() = getConstExprValue(v)) + // In case an array type uses a constant in the same scope as the constexpr variable, // consider it as used. - count(ArrayType at, LocalVariable arrayVariable | - arrayVariable.getType().resolveTypedefs() = at and - v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and - at.getArraySize().toString() = getConstExprValue(v) - ) + countUsesInLocalArraySize(v) } from PotentiallyUnusedLocalVariable v diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index f4607d82cb..92fa3b497f 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -150,3 +150,19 @@ predicate maybeACompileTimeTemplateArgument(Variable v) { ) ) } + +/** Gets the constant value of a constexpr/const variable. */ +private string getConstExprValue(Variable v) { + result = v.getInitializer().getExpr().getValue() and + (v.isConst() or v.isConstexpr()) +} + +/** + * Counts uses of `Variable` v in a local array of size `n` + */ +int countUsesInLocalArraySize(Variable v) { + result = count(ArrayType at, LocalVariable arrayVariable | + arrayVariable.getType().resolveTypedefs() = at and + v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and + at.getArraySize().toString() = getConstExprValue(v)) +} diff --git a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll index 5f3b77e661..1cf4989680 100644 --- a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll +++ b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll @@ -22,25 +22,6 @@ abstract class DeadCodeSharedQuery extends Query { } Query getQuery() { result instanceof DeadCodeSharedQuery } -/** - * Returns integer value of a constexpr variable - */ -int getConstexprValue(Variable v) { - result = v.getInitializer().getExpr().getValue().toInt() and v.isConstexpr() -} - -/** - * Holds if `Variable` v is used for a local array size with value `n` - */ -bindingset[n] -predicate isUsedInLocalArraySize(Variable v, int n) { - // Cf. https://github.com/github/codeql-coding-standards/pull/660/files. - count(ArrayType at, LocalVariable arrayVariable | - arrayVariable.getType().resolveTypedefs() = at and - v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and - at.getArraySize() = n) > 0 -} - /** * Holds if the `Stmt` `s` is either dead or unreachable. */ @@ -72,7 +53,7 @@ predicate isDeadStmt(Stmt s) { va.getTarget() = v and not isDeadOrUnreachableStmt(va.getEnclosingStmt()) ) and - not isUsedInLocalArraySize(v, getConstexprValue(v)) + not (countUsesInLocalArraySize(v) > 0) ) ) ) From 468eabd2c684111f03f7b211dee5bf8054a02611 Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Mon, 16 Sep 2024 20:59:40 +0900 Subject: [PATCH 3/7] Add cases in dead code test where the arrays are constexpr. Complemeting previous case where the integers used for the sizes of the arrays were constexpr. --- cpp/common/test/rules/deadcode/DeadCode.expected | 1 + cpp/common/test/rules/deadcode/test.cpp | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cpp/common/test/rules/deadcode/DeadCode.expected b/cpp/common/test/rules/deadcode/DeadCode.expected index d3252e9401..db68f5f8c4 100644 --- a/cpp/common/test/rules/deadcode/DeadCode.expected +++ b/cpp/common/test/rules/deadcode/DeadCode.expected @@ -14,3 +14,4 @@ | test.cpp:79:17:80:3 | { ... } | This statement is dead code. | | test.cpp:85:3:85:44 | 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. | diff --git a/cpp/common/test/rules/deadcode/test.cpp b/cpp/common/test/rules/deadcode/test.cpp index 982c021563..597c06af43 100644 --- a/cpp/common/test/rules/deadcode/test.cpp +++ b/cpp/common/test/rules/deadcode/test.cpp @@ -86,5 +86,8 @@ int test_dead_code(int x) { constexpr int unused_int{2}; // NON_COMPLIANT - return live5 + live6; // COMPLIANT -} \ No newline at end of file + constexpr int constexpr_used_array[]{3, 4, 5}; // COMPLIANT + constexpr int constexpr_unused_array[]{0, 1, 2}; // NON_COMPLIANT + + return live5 + live6 + constexpr_used_array[1]; // COMPLIANT +} From fb4e41896beb3ad77bb82d1cfd551cf0611eae73 Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Tue, 17 Sep 2024 07:19:25 +0900 Subject: [PATCH 4/7] Refactor get constexpr helper to public in unused variable qll and remove from ql. --- cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql | 6 ------ .../src/codingstandards/cpp/deadcode/UnusedVariables.qll | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql index 0387208514..e89e9ec135 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql @@ -18,12 +18,6 @@ import cpp import codingstandards.cpp.autosar import codingstandards.cpp.deadcode.UnusedVariables -/** Gets the constant value of a constexpr/const variable. */ -private string getConstExprValue(Variable v) { - result = v.getInitializer().getExpr().getValue() and - (v.isConst() or v.isConstexpr()) -} - // This predicate is similar to getUseCount for M0-1-4 except that it also // considers static_asserts. This was created to cater for M0-1-3 specifically // and hence, doesn't attempt to reuse the M0-1-4 specific predicate diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index 92fa3b497f..56d1e2b998 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -152,7 +152,7 @@ predicate maybeACompileTimeTemplateArgument(Variable v) { } /** Gets the constant value of a constexpr/const variable. */ -private string getConstExprValue(Variable v) { +string getConstExprValue(Variable v) { result = v.getInitializer().getExpr().getValue() and (v.isConst() or v.isConstexpr()) } From 3223bcd7c4a47e8b27f280a23b4835b7e2391ac4 Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Tue, 17 Sep 2024 11:36:27 +0900 Subject: [PATCH 5/7] Format ql and test. --- .../codingstandards/cpp/deadcode/UnusedVariables.qll | 10 ++++++---- cpp/common/test/rules/deadcode/DeadCode.expected | 2 +- cpp/common/test/rules/deadcode/test.cpp | 6 +++--- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index 56d1e2b998..912d2babcd 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -161,8 +161,10 @@ string getConstExprValue(Variable v) { * Counts uses of `Variable` v in a local array of size `n` */ int countUsesInLocalArraySize(Variable v) { - result = count(ArrayType at, LocalVariable arrayVariable | - arrayVariable.getType().resolveTypedefs() = at and - v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and - at.getArraySize().toString() = getConstExprValue(v)) + result = + count(ArrayType at, LocalVariable arrayVariable | + arrayVariable.getType().resolveTypedefs() = at and + v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and + at.getArraySize().toString() = getConstExprValue(v) + ) } diff --git a/cpp/common/test/rules/deadcode/DeadCode.expected b/cpp/common/test/rules/deadcode/DeadCode.expected index db68f5f8c4..aec93e0914 100644 --- a/cpp/common/test/rules/deadcode/DeadCode.expected +++ b/cpp/common/test/rules/deadcode/DeadCode.expected @@ -12,6 +12,6 @@ | test.cpp:72:3:73:3 | try { ... } | This statement is dead code. | | test.cpp:73:17:74:3 | { ... } | This statement is dead code. | | test.cpp:79:17:80:3 | { ... } | This statement is dead code. | -| test.cpp:85:3:85:44 | declaration | This statement is dead code. | +| 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. | diff --git a/cpp/common/test/rules/deadcode/test.cpp b/cpp/common/test/rules/deadcode/test.cpp index 597c06af43..d9c0cab277 100644 --- a/cpp/common/test/rules/deadcode/test.cpp +++ b/cpp/common/test/rules/deadcode/test.cpp @@ -81,12 +81,12 @@ int test_dead_code(int x) { static_assert(1); // COMPLIANT - constexpr int constexpr_array_size{6}; // COMPLIANT - int unused_array[constexpr_array_size] {}; // NON_COMPLIANT + constexpr int constexpr_array_size{6}; // COMPLIANT + int unused_array[constexpr_array_size]{}; // NON_COMPLIANT constexpr int unused_int{2}; // NON_COMPLIANT - constexpr int constexpr_used_array[]{3, 4, 5}; // COMPLIANT + constexpr int constexpr_used_array[]{3, 4, 5}; // COMPLIANT constexpr int constexpr_unused_array[]{0, 1, 2}; // NON_COMPLIANT return live5 + live6 + constexpr_used_array[1]; // COMPLIANT From 55cb51debc0f8ee5ed97351892ac6911f532e042 Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Tue, 17 Sep 2024 13:21:20 +0900 Subject: [PATCH 6/7] Add change note --- change_notes/2024-09-17-fix-fp-678-m0-1-9.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change_notes/2024-09-17-fix-fp-678-m0-1-9.md diff --git a/change_notes/2024-09-17-fix-fp-678-m0-1-9.md b/change_notes/2024-09-17-fix-fp-678-m0-1-9.md new file mode 100644 index 0000000000..e068825f4c --- /dev/null +++ b/change_notes/2024-09-17-fix-fp-678-m0-1-9.md @@ -0,0 +1,2 @@ +- `M0-1-9` - `DeadCode.qll`: + - Fixes #678. Remove dead code false positive when integer constant expression is used to define the size of an array. From 234bc05d5fdb566f3beda08adc924b97089f48d1 Mon Sep 17 00:00:00 2001 From: Fernando Jose Date: Wed, 18 Sep 2024 09:29:44 +0900 Subject: [PATCH 7/7] Fix query formatting (remove parentheses around not's argument). --- 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 1cf4989680..2b5be15e80 100644 --- a/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll +++ b/cpp/common/src/codingstandards/cpp/rules/deadcode/DeadCode.qll @@ -53,7 +53,7 @@ predicate isDeadStmt(Stmt s) { va.getTarget() = v and not isDeadOrUnreachableStmt(va.getEnclosingStmt()) ) and - not (countUsesInLocalArraySize(v) > 0) + not countUsesInLocalArraySize(v) > 0 ) ) )