From a3e3f9df72dea181f6629cbee4d1ba84300a8616 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Thu, 15 Feb 2024 12:08:52 -0800 Subject: [PATCH 1/5] Exclude members of uninstantiated templates We can't properly determine the use of uninstantiated template members without looking at all the instantiated templates. Even then, included templates provide an interface that can be partially used, so alerting on those parts that are currently not used doesn't necessarily indicate a programming mistake. --- .../src/codingstandards/cpp/deadcode/UnusedVariables.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index 3437051cac..fa73e81811 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -92,7 +92,9 @@ class PotentiallyUnusedMemberVariable extends MemberVariable { // Must be in a fully defined class, otherwise one of the undefined functions may use the variable getDeclaringType() instanceof FullyDefinedClass and // Lambda captures are not "real" member variables - it's an implementation detail that they are represented that way - not this = any(LambdaCapture lc).getField() + not this = any(LambdaCapture lc).getField() and + // exclude uninstantiated template members + not this.isFromUninstantiatedTemplate(_) } } From ede9e1bcdbaaf096d25fb3e6450ca1d79c1cb11b Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Thu, 15 Feb 2024 12:56:22 -0800 Subject: [PATCH 2/5] Consider possible use as template argument --- .../M0-1-3/UnusedGlobalOrNamespaceVariable.ql | 4 +++- .../src/rules/M0-1-3/UnusedMemberVariable.ql | 4 +++- .../rules/M0-1-3/test_global_or_namespace.cpp | 20 ++++++++++++++++++- cpp/autosar/test/rules/M0-1-3/test_member.cpp | 14 +++++++++++++ .../cpp/deadcode/UnusedVariables.qll | 15 ++++++++++++++ 5 files changed, 54 insertions(+), 3 deletions(-) diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql index 617fbd5f8a..1791755fbe 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql @@ -22,5 +22,7 @@ from PotentiallyUnusedGlobalOrNamespaceVariable v where not isExcluded(v, DeadCodePackage::unusedGlobalOrNamespaceVariableQuery()) and // No variable access - not exists(v.getAnAccess()) + not exists(v.getAnAccess()) and + // Exclude members whose value is compile time and is potentially used to inintialize a template + not maybeACompileTimeTemplateArgument(v) select v, "Variable " + v.getQualifiedName() + " is unused." diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql index 5ebfac4d3f..90d14f61d7 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql @@ -25,5 +25,7 @@ where // No variable access not exists(v.getAnAccess()) and // No explicit initialization in a constructor - not exists(UserProvidedConstructorFieldInit cfi | cfi.getTarget() = v) + not exists(UserProvidedConstructorFieldInit cfi | cfi.getTarget() = v) and + // Exclude members whose value is compile time and is potentially used to inintialize a template + not maybeACompileTimeTemplateArgument(v) select v, "Member variable " + v.getName() + " is unused." diff --git a/cpp/autosar/test/rules/M0-1-3/test_global_or_namespace.cpp b/cpp/autosar/test/rules/M0-1-3/test_global_or_namespace.cpp index 92bb667c45..524830f1b4 100644 --- a/cpp/autosar/test/rules/M0-1-3/test_global_or_namespace.cpp +++ b/cpp/autosar/test/rules/M0-1-3/test_global_or_namespace.cpp @@ -41,4 +41,22 @@ void test_ns() { x2 = 1; } m1(); // ignore dead code in macros } // namespace N1 -int test_access_variable() { return N1::x5; } \ No newline at end of file +int test_access_variable() { return N1::x5; } + +template struct C1 { + int array[t]; // COMPLIANT +}; + +constexpr int g5 = 1; // COMPLIANT - used as template parameter + +namespace ns1 { +constexpr int m1 = 1; // COMPLIANT - used a template parameter +} + +void test_fp_reported_in_384() { + struct C1 l1; + struct C1 l2; + + l1.array[0] = 1; + l2.array[0] = 1; +} \ No newline at end of file diff --git a/cpp/autosar/test/rules/M0-1-3/test_member.cpp b/cpp/autosar/test/rules/M0-1-3/test_member.cpp index 8c0ded8b4e..7aff9a4232 100644 --- a/cpp/autosar/test/rules/M0-1-3/test_member.cpp +++ b/cpp/autosar/test/rules/M0-1-3/test_member.cpp @@ -47,4 +47,18 @@ void test_d() { d.getT(); } +template struct C1 { + int array[t]; // COMPLIANT +}; + +struct C2 { + static constexpr int m1 = 1; // COMPLIANT - used as template parameter +}; + +void test_fp_reported_in_384() { + struct C1 l1; + + l1.array[0] = 1; +} + } // namespace test \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index fa73e81811..cda009558d 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -1,5 +1,6 @@ import cpp import codingstandards.cpp.FunctionEquivalence +import codingstandards.cpp.Scope /** * A type that contains a template parameter type (doesn't count pointers or references). @@ -121,3 +122,17 @@ class UserProvidedConstructorFieldInit extends ConstructorFieldInit { not getEnclosingFunction().isCompilerGenerated() } } + +predicate maybeACompileTimeTemplateArgument(Variable v) { + v.isConstexpr() and + exists(ClassTemplateInstantiation cti, TranslationUnit tu | + cti.getATemplateArgument().(Expr).getValue() = v.getInitializer().getExpr().getValue() and + ( + cti.getFile() = tu and + ( + v.getADeclarationEntry().getFile() = tu or + tu.getATransitivelyIncludedFile() = v.getADeclarationEntry().getFile() + ) + ) + ) +} From 96a6358f5e6365ee265c213fd84256b25f015fe6 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Thu, 15 Feb 2024 13:01:14 -0800 Subject: [PATCH 3/5] Adjust alert to follow styleguide --- .../rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql | 2 +- cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql | 2 +- cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql | 2 +- .../M0-1-3/UnusedGlobalOrNamespaceVariable.expected | 8 ++++---- .../test/rules/M0-1-3/UnusedLocalVariable.expected | 12 ++++++------ .../test/rules/M0-1-3/UnusedMemberVariable.expected | 8 ++++---- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql index 1791755fbe..ba6b6df20a 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.ql @@ -25,4 +25,4 @@ where not exists(v.getAnAccess()) and // Exclude members whose value is compile time and is potentially used to inintialize a template not maybeACompileTimeTemplateArgument(v) -select v, "Variable " + v.getQualifiedName() + " is unused." +select v, "Variable '" + v.getQualifiedName() + "' is unused." diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql index 9ac58a6de8..f088bb1b74 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedLocalVariable.ql @@ -50,4 +50,4 @@ where // Local variable is never accessed not exists(v.getAnAccess()) and getUseCountConservatively(v) = 0 -select v, "Local variable " + v.getName() + " in " + v.getFunction().getName() + " is not used." +select v, "Local variable '" + v.getName() + "' in '" + v.getFunction().getName() + "' is not used." diff --git a/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql b/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql index 90d14f61d7..a27f9cbcab 100644 --- a/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql +++ b/cpp/autosar/src/rules/M0-1-3/UnusedMemberVariable.ql @@ -28,4 +28,4 @@ where not exists(UserProvidedConstructorFieldInit cfi | cfi.getTarget() = v) and // Exclude members whose value is compile time and is potentially used to inintialize a template not maybeACompileTimeTemplateArgument(v) -select v, "Member variable " + v.getName() + " is unused." +select v, "Member variable '" + v.getName() + "' is unused." diff --git a/cpp/autosar/test/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.expected b/cpp/autosar/test/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.expected index 8ee5d76bfa..97c3d17a84 100644 --- a/cpp/autosar/test/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.expected +++ b/cpp/autosar/test/rules/M0-1-3/UnusedGlobalOrNamespaceVariable.expected @@ -1,4 +1,4 @@ -| test_global_or_namespace.cpp:3:5:3:6 | g3 | Variable g3 is unused. | -| test_global_or_namespace.cpp:18:4:18:4 | a | Variable a is unused. | -| test_global_or_namespace.cpp:26:5:26:6 | x3 | Variable N1::x3 is unused. | -| test_global_or_namespace.cpp:36:5:36:5 | a | Variable N1::a is unused. | +| test_global_or_namespace.cpp:3:5:3:6 | g3 | Variable 'g3' is unused. | +| test_global_or_namespace.cpp:18:4:18:4 | a | Variable 'a' is unused. | +| test_global_or_namespace.cpp:26:5:26:6 | x3 | Variable 'N1::x3' is unused. | +| test_global_or_namespace.cpp:36:5:36:5 | a | Variable 'N1::a' is unused. | diff --git a/cpp/autosar/test/rules/M0-1-3/UnusedLocalVariable.expected b/cpp/autosar/test/rules/M0-1-3/UnusedLocalVariable.expected index 0d6f7de28b..77eb030716 100644 --- a/cpp/autosar/test/rules/M0-1-3/UnusedLocalVariable.expected +++ b/cpp/autosar/test/rules/M0-1-3/UnusedLocalVariable.expected @@ -1,6 +1,6 @@ -| test.cpp:7:7:7:7 | y | Local variable y in test_simple is not used. | -| test.cpp:14:13:14:13 | y | Local variable y in test_const is not used. | -| test.cpp:17:7:17:7 | z | Local variable z in test_const is not used. | -| test.cpp:23:5:23:5 | t | Local variable t in f1 is not used. | -| test.cpp:23:5:23:5 | t | Local variable t in f1 is not used. | -| test.cpp:44:6:44:6 | a | Local variable a in test_side_effect_init is not used. | +| test.cpp:7:7:7:7 | y | Local variable 'y' in 'test_simple' is not used. | +| test.cpp:14:13:14:13 | y | Local variable 'y' in 'test_const' is not used. | +| test.cpp:17:7:17:7 | z | Local variable 'z' in 'test_const' is not used. | +| test.cpp:23:5:23:5 | t | Local variable 't' in 'f1' is not used. | +| test.cpp:23:5:23:5 | t | Local variable 't' in 'f1' is not used. | +| test.cpp:44:6:44:6 | a | Local variable 'a' in 'test_side_effect_init' is not used. | diff --git a/cpp/autosar/test/rules/M0-1-3/UnusedMemberVariable.expected b/cpp/autosar/test/rules/M0-1-3/UnusedMemberVariable.expected index 14e0cb42ee..e424945d5b 100644 --- a/cpp/autosar/test/rules/M0-1-3/UnusedMemberVariable.expected +++ b/cpp/autosar/test/rules/M0-1-3/UnusedMemberVariable.expected @@ -1,4 +1,4 @@ -| test_member.cpp:4:7:4:8 | m1 | Member variable m1 is unused. | -| test_member.cpp:17:9:17:11 | pad | Member variable pad is unused. | -| test_member.cpp:19:9:19:11 | sm2 | Member variable sm2 is unused. | -| test_member.cpp:31:7:31:8 | m1 | Member variable m1 is unused. | +| test_member.cpp:4:7:4:8 | m1 | Member variable 'm1' is unused. | +| test_member.cpp:17:9:17:11 | pad | Member variable 'pad' is unused. | +| test_member.cpp:19:9:19:11 | sm2 | Member variable 'sm2' is unused. | +| test_member.cpp:31:7:31:8 | m1 | Member variable 'm1' is unused. | From e55a5b31dc08800088bec14442843099ec654bb6 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Thu, 15 Feb 2024 13:04:37 -0800 Subject: [PATCH 4/5] Add changenote --- change_notes/2024-02-15-fix-fp-m0-1-3.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 change_notes/2024-02-15-fix-fp-m0-1-3.md diff --git a/change_notes/2024-02-15-fix-fp-m0-1-3.md b/change_notes/2024-02-15-fix-fp-m0-1-3.md new file mode 100644 index 0000000000..e84f9fb6db --- /dev/null +++ b/change_notes/2024-02-15-fix-fp-m0-1-3.md @@ -0,0 +1,4 @@ +- `M0-1-3` - `UnusedMemberVariable.ql`, `UnusedGlobalOrNamespaceVariable.ql`: + - Address FP reported in #384. Exclude variables with compile time values that may have been used as a template argument. + - Exclude uninstantiated template members. + - Reformat the alert message to adhere to the style-guide. From 5df06ecb6ab3e6719098671aa9be2a957f1b7d22 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Thu, 15 Feb 2024 13:09:40 -0800 Subject: [PATCH 5/5] Add qldoc to `maybeACompileTimeTemplateArgument` --- .../src/codingstandards/cpp/deadcode/UnusedVariables.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index cda009558d..077c35a2aa 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -123,6 +123,10 @@ class UserProvidedConstructorFieldInit extends ConstructorFieldInit { } } +/** + * Holds if `v` may hold a compile time value and is accessible to a template instantiation that + * receives a constant value as an argument equal to the value of `v`. + */ predicate maybeACompileTimeTemplateArgument(Variable v) { v.isConstexpr() and exists(ClassTemplateInstantiation cti, TranslationUnit tu |