From 1340cdbcef49a5f7b99fa65370a5fef97e5977db Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 24 Oct 2024 23:23:50 -0700 Subject: [PATCH 1/8] Implement RULE-2-8, project should not contain unused object definitions. Also add a new AlertReporting shared query library for deduplicating results across macro definitions/invocations/etc. Split __attribute__((unused)) variables (and similar) to a Strict pair of queries. --- .../rules/RULE-2-8/UnusedObjectDefinition.ql | 24 ++ .../RULE-2-8/UnusedObjectDefinitionInMacro.ql | 24 ++ .../UnusedObjectDefinitionInMacroStrict.ql | 27 ++ .../RULE-2-8/UnusedObjectDefinitionStrict.ql | 26 ++ .../RULE-2-8/UnusedObjectDefinition.expected | 8 + .../RULE-2-8/UnusedObjectDefinition.qlref | 1 + .../UnusedObjectDefinitionInMacro.expected | 2 + .../UnusedObjectDefinitionInMacro.qlref | 1 + ...usedObjectDefinitionInMacroStrict.expected | 2 + .../UnusedObjectDefinitionInMacroStrict.qlref | 1 + .../UnusedObjectDefinitionStrict.expected | 2 + .../UnusedObjectDefinitionStrict.qlref | 1 + c/misra/test/rules/RULE-2-8/test.c | 113 ++++++ .../codingstandards/cpp/AlertReporting.qll | 28 +- .../DeduplicateMacroResults.qll | 379 ++++++++++++++++++ .../cpp/deadcode/UnusedObjects.qll | 176 ++++++++ .../cpp/exclusions/c/DeadCode2.qll | 78 ++++ .../cpp/exclusions/c/RuleMetadata.qll | 3 + .../DeduplicateMacroResults.expected | 6 + .../alertreporting/DeduplicateMacroResults.ql | 32 ++ .../deduplicatemacroresults.cpp | 53 +++ rule_packages/c/DeadCode2.json | 66 +++ 22 files changed, 1050 insertions(+), 3 deletions(-) create mode 100644 c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql create mode 100644 c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql create mode 100644 c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql create mode 100644 c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.qlref create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected create mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.qlref create mode 100644 c/misra/test/rules/RULE-2-8/test.c create mode 100644 cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll create mode 100644 cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll create mode 100644 cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected create mode 100644 cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql create mode 100644 cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp create mode 100644 rule_packages/c/DeadCode2.json diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql new file mode 100644 index 0000000000..420733d4ac --- /dev/null +++ b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql @@ -0,0 +1,24 @@ +/** + * @id c/misra/unused-object-definition + * @name RULE-2-8: A project should not contain unused object definitions + * @description Object definitions which are unused should be removed. + * @kind problem + * @precision very-high + * @problem.severity recommendation + * @tags external/misra/id/rule-2-8 + * maintainability + * performance + * external/misra/c/2012/amendment4 + * external/misra/obligation/advisory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.deadcode.UnusedObjects + +from ReportDeadObjectAtDefinition report +where + not isExcluded(report.getPrimaryElement(), DeadCode2Package::unusedObjectDefinitionQuery()) and + not report.hasAttrUnused() +select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), + report.getOptionalPlaceholderMessage() diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql new file mode 100644 index 0000000000..d5c339c157 --- /dev/null +++ b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql @@ -0,0 +1,24 @@ +/** + * @id c/misra/unused-object-definition-in-macro + * @name RULE-2-8: Project macros should not include unused object definitions + * @description Macros should not have unused object definitions. + * @kind problem + * @precision very-high + * @problem.severity recommendation + * @tags external/misra/id/rule-2-8 + * maintainability + * performance + * external/misra/c/2012/amendment4 + * external/misra/obligation/advisory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.deadcode.UnusedObjects + +from ReportDeadObjectInMacro report +where + not isExcluded(report.getPrimaryElement(), DeadCode2Package::unusedObjectDefinitionInMacroQuery()) and + not report.hasAttrUnused() +select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), + report.getOptionalPlaceholderMessage() diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql new file mode 100644 index 0000000000..7eead60424 --- /dev/null +++ b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql @@ -0,0 +1,27 @@ +/** + * @id c/misra/unused-object-definition-in-macro-strict + * @name RULE-2-8: Project macros should not include '__attribute__((unused))' object definitions + * @description A strict query which reports all unused object definitions in macros with + * '__attribute__((unused))'. + * @kind problem + * @precision very-high + * @problem.severity recommendation + * @tags external/misra/id/rule-2-8 + * maintainability + * performance + * external/misra/c/2012/amendment4 + * external/misra/c/strict + * external/misra/obligation/advisory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.deadcode.UnusedObjects + +from ReportDeadObjectInMacro report +where + not isExcluded(report.getPrimaryElement(), + DeadCode2Package::unusedObjectDefinitionInMacroStrictQuery()) and + report.hasAttrUnused() +select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), + report.getOptionalPlaceholderMessage() diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql new file mode 100644 index 0000000000..ad92c79481 --- /dev/null +++ b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql @@ -0,0 +1,26 @@ +/** + * @id c/misra/unused-object-definition-strict + * @name RULE-2-8: A project should not contain '__attribute__((unused))' object definitions + * @description A strict query which reports all unused object definitions with + * '__attribute__((unused))'. + * @kind problem + * @precision very-high + * @problem.severity recommendation + * @tags external/misra/id/rule-2-8 + * maintainability + * performance + * external/misra/c/2012/amendment4 + * external/misra/c/strict + * external/misra/obligation/advisory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.deadcode.UnusedObjects + +from ReportDeadObjectAtDefinition report +where + not isExcluded(report.getPrimaryElement(), DeadCode2Package::unusedObjectDefinitionStrictQuery()) and + report.hasAttrUnused() +select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), + report.getOptionalPlaceholderMessage() diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected new file mode 100644 index 0000000000..fc6f320539 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected @@ -0,0 +1,8 @@ +| test.c:6:5:6:6 | definition of g2 | Unused object definition 'g2'. | test.c:6:5:6:6 | test.c:6:5:6:6 | | +| test.c:9:5:9:6 | definition of g3 | Unused object definition 'g3'. | test.c:9:5:9:6 | test.c:9:5:9:6 | | +| test.c:20:7:20:8 | definition of l2 | Unused object definition 'l2'. | test.c:20:7:20:8 | test.c:20:7:20:8 | | +| test.c:27:7:27:8 | definition of l5 | Unused object definition 'l5'. | test.c:27:7:27:8 | test.c:27:7:27:8 | | +| test.c:37:10:37:11 | definition of g5 | Unused object definition 'g5'. | test.c:37:10:37:11 | test.c:37:10:37:11 | | +| test.c:45:9:45:10 | definition of g6 | Unused object definition 'g6'. | test.c:45:9:45:10 | test.c:45:9:45:10 | | +| test.c:51:5:51:6 | definition of g7 | Unused object definition 'g7'. | test.c:51:5:51:6 | test.c:51:5:51:6 | | +| test.c:64:3:64:18 | ONLY_DEF_VAR(x) | Unused object definition 'l2' from macro '$@'. | test.c:60:1:60:34 | test.c:60:1:60:34 | ONLY_DEF_VAR | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.qlref b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.qlref new file mode 100644 index 0000000000..096c4c64f1 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.qlref @@ -0,0 +1 @@ +rules/RULE-2-8/UnusedObjectDefinition.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected new file mode 100644 index 0000000000..c25c136789 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected @@ -0,0 +1,2 @@ +| test.c:68:1:71:5 | #define ALSO_DEF_VAR(x) int x = 0; while (1) ; | Macro 'ALSO_DEF_VAR' defines unused object of varied names, for example, '$@'. | test.c:73:16:73:17 | test.c:73:16:73:17 | l1 | +| test.c:77:1:82:3 | #define DEF_UNUSED_INNER_VAR() { int _v = 0; while (1) ; } | Macro 'DEF_UNUSED_INNER_VAR' defines unused object '_v'. | test.c:77:1:82:3 | test.c:77:1:82:3 | (ignored) | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref new file mode 100644 index 0000000000..057e684fd0 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref @@ -0,0 +1 @@ +rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected new file mode 100644 index 0000000000..2919c65cb7 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected @@ -0,0 +1,2 @@ +| test.c:94:1:97:5 | #define ALSO_DEF_ATTR_UNUSED_VAR(x) __attribute__((unused)) int x = 0; while (1) ; | Macro 'ALSO_DEF_ATTR_UNUSED_VAR' defines unused object of varied names, for example, '$@'. | test.c:99:28:99:29 | test.c:99:28:99:29 | l1 | +| test.c:104:1:109:3 | #define DEF_ATTR_UNUSED_INNER_VAR() { __attribute__((unused)) int _v = 0; while (1) ; } | Macro 'DEF_ATTR_UNUSED_INNER_VAR' defines unused object '_v'. | test.c:104:1:109:3 | test.c:104:1:109:3 | (ignored) | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref new file mode 100644 index 0000000000..f04653dcb6 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref @@ -0,0 +1 @@ +rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected new file mode 100644 index 0000000000..624368ac54 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected @@ -0,0 +1,2 @@ +| test.c:87:29:87:30 | definition of g8 | Unused object definition 'g8'. | test.c:87:29:87:30 | test.c:87:29:87:30 | | +| test.c:90:3:90:30 | ONLY_DEF_ATTR_UNUSED_VAR(x) | Unused object definition 'l2' from macro '$@'. | test.c:88:1:88:70 | test.c:88:1:88:70 | ONLY_DEF_ATTR_UNUSED_VAR | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.qlref b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.qlref new file mode 100644 index 0000000000..4aa7269881 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.qlref @@ -0,0 +1 @@ +rules/RULE-2-8/UnusedObjectDefinitionStrict.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-8/test.c b/c/misra/test/rules/RULE-2-8/test.c new file mode 100644 index 0000000000..21a2479163 --- /dev/null +++ b/c/misra/test/rules/RULE-2-8/test.c @@ -0,0 +1,113 @@ +// Not a definition, only a declaration: +extern int g1; // COMPLIANT + +// Both declared + defined: +extern int g2; // COMPLIANT +int g2 = 1; // NON_COMPLIANT + +// Definition is only declaration: +int g3 = 1; // NON_COMPLIANT + +// Definition, but value is required for program to compile: +int g4 = 1; // COMPLIANT +void f1() { g4; } + +// Local variables: +void f2() { + int l1; // COMPLIANT + l1; + + int l2; // NON-COMPLIANT + + // Value is required for the program to compile: + int l3; // COMPLIANT + sizeof(l3); + + int l4, // COMPLIANT + l5; // NON-COMPLIANT + l4; +} + +// Struct fields are not objects: +struct s { + int x; // COMPLIANT +}; + +// Declaration of type struct is an object: +struct s g5; // NON-COMPLIANT + +// Struct fields are not objects: +union u { + int x; // COMPLIANT +}; + +// Declaration of type union is an object: +union u g6; // NON-COMPLIANT + +// Typedefs are not objects: +typedef int td1; // COMPLIANT + +// Declaration of typedef type object: +td1 g7; // NON-COMPLIANT + +// Function parameters are not objects: +void f3(int p) {} // COMPLIANT + +// Function type parameters are not objects: +typedef int td2(int x); // COMPLIANT + +// Macros that define unused vars tests: +#define ONLY_DEF_VAR(x) int x = 0; +void f4() { + ONLY_DEF_VAR(l1); // COMPLIANT + l1; + ONLY_DEF_VAR(l2); // NON-COMPLIANT +} + +// NON-COMPLIANT +#define ALSO_DEF_VAR(x) \ + int x = 0; \ + while (1) \ + ; +void f5() { + ALSO_DEF_VAR(l1); // COMPLIANT + ALSO_DEF_VAR(l2); // COMPLIANT +} + +#define DEF_UNUSED_INNER_VAR() \ + { \ + int _v = 0; \ + while (1) \ + ; \ + } // NON-COMPLIANT +void f6() { + DEF_UNUSED_INNER_VAR(); // COMPLIANT +} + +__attribute__((unused)) int g8 = 1; // NON-COMPLIANT +#define ONLY_DEF_ATTR_UNUSED_VAR(x) __attribute__((unused)) int x = 0; +void f7() { + ONLY_DEF_ATTR_UNUSED_VAR(l2); // NON-COMPLIANT +} + +// NON-COMPLIANT +#define ALSO_DEF_ATTR_UNUSED_VAR(x) \ + __attribute__((unused)) int x = 0; \ + while (1) \ + ; +void f8() { + ALSO_DEF_ATTR_UNUSED_VAR(l1); // COMPLIANT + ALSO_DEF_ATTR_UNUSED_VAR(l2); // COMPLIANT +} + +// NON-COMPLIANT +#define DEF_ATTR_UNUSED_INNER_VAR() \ + { \ + __attribute__((unused)) int _v = 0; \ + while (1) \ + ; \ + } + +void f9() { + DEF_ATTR_UNUSED_INNER_VAR(); // COMPLIANT +} diff --git a/cpp/common/src/codingstandards/cpp/AlertReporting.qll b/cpp/common/src/codingstandards/cpp/AlertReporting.qll index 4259e1b67d..3ef5315906 100644 --- a/cpp/common/src/codingstandards/cpp/AlertReporting.qll +++ b/cpp/common/src/codingstandards/cpp/AlertReporting.qll @@ -18,19 +18,24 @@ module MacroUnwrapper { } /** - * Gets the primary macro that generated the result element. + * Gets the primary macro invocation that generated the result element. */ - Macro getPrimaryMacro(ResultElement re) { + MacroInvocation getPrimaryMacroInvocation(ResultElement re) { exists(MacroInvocation mi | mi = getAMacroInvocation(re) and // No other more specific macro that expands to element not exists(MacroInvocation otherMi | otherMi = getAMacroInvocation(re) and otherMi.getParentInvocation() = mi ) and - result = mi.getMacro() + result = mi ) } + /** + * Gets the primary macro that generated the result element. + */ + Macro getPrimaryMacro(ResultElement re) { result = getPrimaryMacroInvocation(re).getMacro() } + /** * If a result element is expanded from a macro invocation, then return the "primary" macro that * generated the element, otherwise return the element itself. @@ -38,4 +43,21 @@ module MacroUnwrapper { Element unwrapElement(ResultElement re) { if exists(getPrimaryMacro(re)) then result = getPrimaryMacro(re) else result = re } + + /* Final class so we can extend it */ + final private class FinalMacroInvocation = MacroInvocation; + + /* A macro invocation that expands to create a `ResultElement` */ + class ResultMacroExpansion extends FinalMacroInvocation { + ResultElement re; + + ResultMacroExpansion() { re = getAnExpandedElement() } + + ResultElement getResultElement() { result = re } + } + + /* The most specific macro invocation that expands to create this `ResultElement`. */ + class PrimaryMacroExpansion extends ResultMacroExpansion { + PrimaryMacroExpansion() { this = getPrimaryMacroInvocation(re) } + } } diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll new file mode 100644 index 0000000000..7c4e8ef41d --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll @@ -0,0 +1,379 @@ +import codingstandards.cpp.AlertReporting + +/** + * A configuration for deduplicating query results inside of macros. + * + * See doc comment on `DeduplicateMacroResults` module. + */ +signature module DeduplicateMacroConfigSig { + /** + * Stringify the `ResultElement`. All `ResultElement`s that share an "identity" should stringify + * to the same string to get proper results. + */ + string describe(ResultElement re); +} + +/** + * A configuration for generating reports from reports that may or may not be duplicated across + * macro expansions. + * + * See doc comment on `DeduplicateMacroResults` module. + * + * This signature is used to parameterize the module `DeduplicateMacroResults::Report`. + */ +signature module MacroReportConfigSig { + /* Create a message to describe this macro, with a string describing its `ResultElement`. */ + bindingset[description] + string getMessageSameResultInAllExpansions(Macro m, string description); + + /* Create a message to describe this macro, using '$@' to describe an example `ResultElement`. */ + string getMessageVariedResultInAllExpansions(Macro m); + + /* + * Create a message to describe this macro expansion which produces a `ResultElement`, using '$@' + * to describe the relevant macro. + */ + + string getMessageResultInIsolatedExpansion(ResultElement element); +} + +/** + * A module for taking the results of `MacroUnwrapper` and consolidating them. + * + * The module `MacroUnwrapper` is great for simple alerts such as usage of banned functions. In + * such cases, reporting "call to 'foo()' in macro 'M'" will only have one result even if the macro + * is expanded multiple times. + * + * However, other queries may have a dynamic message which can vary per macro call site due to + * token manipulation (`a ## b`), for instance, "Macro 'M' defines unused object 'generated_name_x'" + * which will lead to hundreds of results if there are hundreds of such generated names. + * + * This module can be used to find and easily report non-compliant behavior, grouped by the macro + * it originates in, regardless of whether the messages will exactly match. + * + * ## General Usage + * + * To use this macro, define a class for the relevant behavior, and a means of stringifying + * relevant elements as a config, to parameterize the `DeduplicateMacroResults` module. + * + * ``` + * class InvalidFoo extends Foo { + * InvalidFoo() { ... } + * } + * + * module DeduplicateFooInMacrosConfig implements DeduplicateMacroConfigSig { + * string describe(InvalidFoo foo) { result = ... } + * } + * + * import DeduplicateMacroResults as DeduplicateFooInMacros; + * ``` + * + * This module exposes the following classes: + * - `PrimaryMacroSameResultElementInAllInvocations extends Macro`: Every invocation of this macro + * generates an `InvalidFoo` which stringifies to the same thing. Use the method + * `getResultElementDescription()` to get that shared string. + * - `PrimaryMacroDifferentResultElementInAllInvocations extends Macro`: Every invocation of this + * macro generates an `InvalidFoo`, but they do not all stringify to the same thing. Use the + * method `getExampleResultElement()` to get an *single* example `InvalidFoo` to help users fix + * and understand the issue. + * - `IsolatedMacroExpansionWithResultElement extends MacroInvocation`: An invocation of a macro + * which in this particular instance generates an `InvalidFoo`, while other invocations of the + * same macro do not. + * + * The three above classes all attempt to resolve to the most *specific* macro to the issue at + * hand. For instance, if macro `M1` calls macro `M2` which expands to an `InvalidFoo`, then the + * problem may be with `M2` (it is the most specific macro call here), or the problem may be with + * `M2` (if all expansions of `M2` generate an `InvalidFoo` but not all expansions of `M1` do so). + * + * ## Generating Report Objects + * + * This module also can be used to more easily report issues across these cases, by implementing + * `MacroReportConfigSig` and importing `DeduplicateMacroResults::Report::ReportResultInMacro`. + * + * ``` + * module InvalidFooInMacroReportConfig implements MacroReportConfigSig { + * + * // ***Take care that usage of $@ is correct in the following predicates***!!!! + * bindingset[description] + * string getMessageSameResultInAllExpansions(Macro m, string description) { + * result = "Macro " + m.getName() + " always has invalid foo " + description + * } + * + * string getMessageVariedResultInAllExpansions(Macro m) { + * result = "Macro " + m.getName() + " always has invalid foo, for example '$@'." + * } + * + * string getMessageResultInIsolatedExpansion(InvalidFoo foo) { + * result = "Invocation of macro $@ has invalid foo '" + foo.getName() + "'." + * } + * } + * + * import DeduplicateFooInMacros::Report as Report; + * + * from Report::ReportResultInMacro report + * where not excluded(report.getPrimaryElement(), ...) + * select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), + * report.getOptionalPlaceholderMessage() + * ``` + * + * Note that this does *not* (currently) generate a result for elements not contained by a macro. + * To do report such cases, either add support for that in this module, or write a separate query + * that reports `InvalidFoo` cases where not `.isInMacroExpansion()`. + */ +module DeduplicateMacroResults< + ResultType ResultElement, DeduplicateMacroConfigSig Config> +{ + /* A final class so that we may extend Macro. */ + final private class FinalMacro = Macro; + + /* Helper final class import so that we may reference/extend it. */ + final private class ResultMacroExpansion = MacroUnwrapper::ResultMacroExpansion; + + /** + * A macro for which all of its invocations produce an element that is described the same way. + * + * This is not necessarily the "primary" / most specific macro for these result elements. + * This difference is captured in `PrimarySameResultElementInAllMacroInvocations`, and the two + * classes are only separate to avoid non-monotonic recursion. + */ + private class SameResultElementInAllMacroInvocations extends FinalMacro { + string resultElementDescription; + + SameResultElementInAllMacroInvocations() { + forex(MacroInvocation mi | mi = getAnInvocation() | + Config::describe(mi.(ResultMacroExpansion).getResultElement()) = resultElementDescription + ) + } + + string getResultElementDescription() { result = resultElementDescription } + + ResultElement getAResultElement() { + result = getAnInvocation().(ResultMacroExpansion).getResultElement() + } + } + + /** + * A macro for which all of its invocations produce an element that is described the same way. + * + * This is the necessarily the "primary" / most specific macro for these result elements. + */ + class PrimaryMacroSameResultElementInAllInvocations extends SameResultElementInAllMacroInvocations + { + PrimaryMacroSameResultElementInAllInvocations() { + not exists(MacroInvocation inner | + inner.getParentInvocation() = getAnInvocation() and + inner.getMacro() instanceof SameResultElementInAllMacroInvocations + ) + } + } + + /** + * A expansion that generates a `ResultElement` that is uniquely described by the config. + * + * This is used so that we can find a single example invocation site to report as an example for + * macros which generate an array of different `ResultElement`s that are described differently. + * + * For example, two macro invocations may be given the same arguments, and generate the same + * `ResultElement`, while a third macro invocation is unique and generates a unique + * `ResultElement`. We wish to direct the user to that unique example or we will show the user + * two different reports for one underlying issue. + */ + private class UniqueResultMacroExpansion extends ResultMacroExpansion { + UniqueResultMacroExpansion() { + not exists(ResultMacroExpansion other | + not this = other and + this.getMacroName() = other.getMacroName() and + Config::describe(this.getResultElement()) = Config::describe(other.getResultElement()) + ) + } + } + + /** + * A macro for which all of its invocations produce an element, but they are not all described the + * same way. + * + * This is not necessarily the "primary" / most specific macro for these result elements. + * This difference is captured in `PrimaryDiferentResultElementInAllMacroInvocations`, and the two + * classes are only separate to avoid non-monotonic recursion. + */ + private class DifferentResultElementInAllMacroInvocations extends FinalMacro { + ResultElement exampleResultElement; + + DifferentResultElementInAllMacroInvocations() { + forex(MacroInvocation mi | mi = getAnInvocation() | mi instanceof ResultMacroExpansion) and + count(getAnInvocation().(ResultMacroExpansion).getResultElement()) > 1 and + exists(string description | + description = + rank[1](Config::describe(getAnInvocation().(UniqueResultMacroExpansion).getResultElement()) + ) and + Config::describe(exampleResultElement) = description and + exampleResultElement = getAnInvocation().(ResultMacroExpansion).getResultElement() + ) + } + + ResultElement getExampleResultElement() { result = exampleResultElement } + + ResultElement getAResultElement() { + result = getAnInvocation().(ResultMacroExpansion).getResultElement() + } + } + + /** + * A macro for which all of its invocations produce an element, but they are not all described the + * same way. + * + * This is "primary" / most specific macro for these result elements. + */ + class PrimaryMacroDifferentResultElementInAllInvocations extends DifferentResultElementInAllMacroInvocations + { + PrimaryMacroDifferentResultElementInAllInvocations() { + not exists(MacroInvocation inner | + inner.getParentInvocation() = getAnInvocation() and + inner.getMacro() instanceof DifferentResultElementInAllMacroInvocations + ) + } + } + + /* + * Convenience predicate to know when invalid macro expansions have been reported at their macro + * definition. + */ + + private predicate reported(Macro macro) { + macro instanceof PrimaryMacroSameResultElementInAllInvocations or + macro instanceof PrimaryMacroDifferentResultElementInAllInvocations + } + + /** + * A macro invocation for which the target macro does not always produce a `ResultElement`, but + * this specific invocation of it does. + * + * This is "primary" / most specific macro for these result elements. It will also does not match + * `MacroInvocation`s inside of a `MacroInvocation` of a `Macro` which always produces a + * `ResultElement`, indicating that the real problem lies with that other `Macro` instead of with + * this particular invocation. + */ + class IsolatedMacroExpansionWithResultElement extends ResultMacroExpansion { + IsolatedMacroExpansionWithResultElement() { + not reported(getParentInvocation*().getMacro()) and + not exists(MacroInvocation inner | + reported(inner.getMacro()) and + inner.getParentInvocation*() = this + ) and + not exists(ResultMacroExpansion moreSpecific | + moreSpecific.getResultElement() = getResultElement() and + moreSpecific.getParentInvocation+() = this + ) + } + } + + /** + * A module for generating reports across the various cases of problematic macros, problematic + * macro invocations. + * + * See the doc comment for the `DeduplicateMacroResults` module for more info. + */ + module Report ReportConfig> { + newtype TReportResultInMacro = + TReportMacroResultWithSameName(PrimaryMacroSameResultElementInAllInvocations def) or + TReportMacroResultWithVariedName(PrimaryMacroDifferentResultElementInAllInvocations def) or + TReportIsolatedMacroResult(IsolatedMacroExpansionWithResultElement def) + + /** + * An instance of a `ResultElement` to be reported to a user. + * + * To show a report, use the following methods: + * - `report.getPrimaryElement()` + * - `report.getMessage()` + * - `report.getOptionalPlaceholderLocation()` + * - `report.getOptionalPlaceholderMessage()` + * + * The values returned by these methods are configured by the `MacroReportConfigSig` + * signature parameter. + */ + class ReportResultInMacro extends TReportResultInMacro { + string toString() { result = getMessage() } + + string getMessage() { + exists(PrimaryMacroDifferentResultElementInAllInvocations def | + this = TReportMacroResultWithVariedName(def) and + result = ReportConfig::getMessageVariedResultInAllExpansions(def) + ) + or + exists(PrimaryMacroSameResultElementInAllInvocations def | + this = TReportMacroResultWithSameName(def) and + result = + ReportConfig::getMessageSameResultInAllExpansions(def, def.getResultElementDescription()) + ) + or + exists(IsolatedMacroExpansionWithResultElement def | + this = TReportIsolatedMacroResult(def) and + result = ReportConfig::getMessageResultInIsolatedExpansion(def.getResultElement()) + ) + } + + Element getPrimaryElement() { + this = TReportMacroResultWithSameName(result) + or + this = TReportMacroResultWithVariedName(result) + or + this = TReportIsolatedMacroResult(result) + } + + Location getOptionalPlaceholderLocation() { + exists(PrimaryMacroDifferentResultElementInAllInvocations def | + this = TReportMacroResultWithVariedName(def) and + result = def.getExampleResultElement().getLocation() + ) + or + exists(PrimaryMacroSameResultElementInAllInvocations def | + this = TReportMacroResultWithSameName(def) and + result = def.getLocation() + ) + or + exists(IsolatedMacroExpansionWithResultElement def | + this = TReportIsolatedMacroResult(def) and + result = def.getMacro().getLocation() + ) + } + + string getOptionalPlaceholderMessage() { + exists(PrimaryMacroDifferentResultElementInAllInvocations def | + this = TReportMacroResultWithVariedName(def) and + result = Config::describe(def.getExampleResultElement()) + ) + or + this = TReportMacroResultWithSameName(_) and + result = "(ignored)" + or + this = TReportIsolatedMacroResult(_) and + result = getMacro().getName() + } + + Macro getMacro() { + this = TReportMacroResultWithVariedName(result) + or + this = TReportMacroResultWithSameName(result) + or + exists(IsolatedMacroExpansionWithResultElement def | + this = TReportIsolatedMacroResult(def) and + result = def.getMacro() + ) + } + + ResultMacroExpansion getAResultMacroExpansion() { + exists(PrimaryMacroDifferentResultElementInAllInvocations def | + this = TReportMacroResultWithVariedName(def) and + result = def.getAnInvocation() + ) + or + exists(PrimaryMacroSameResultElementInAllInvocations def | + this = TReportMacroResultWithSameName(def) and + result = def.getAnInvocation() + ) + or + this = TReportIsolatedMacroResult(result) + } + } + } +} diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll new file mode 100644 index 0000000000..70944dfad4 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll @@ -0,0 +1,176 @@ +import cpp +import codingstandards.cpp.alertreporting.HoldsForAllCopies +import codingstandards.cpp.alertreporting.DeduplicateMacroResults + +/** + * An unused object definition is an object, meaning a place in memory, whose definition could be + * removed and the program would still compile. + * + * Technically, parameters may be considered objects, but they are covered by their own rule. + * Similarly, members of structs are an addressable place in memory, and may be considered objects. + * However, the member declaration is nothing but a layout offset, which is not an object. + * + * This therefore only reports variables (local or top level) which have a definition, and are + * unused. + */ +class UnusedObjectDefinition extends VariableDeclarationEntry { + UnusedObjectDefinition() { + not exists(VariableAccess access | access.getTarget() = getVariable()) and + getVariable().getDefinition() = this and + not this instanceof ParameterDeclarationEntry and + not getVariable() instanceof MemberVariable + } + + /* Dead objects with these attributes are reported in the "strict" queries. */ + predicate hasAttrUnused() { + getVariable().getAnAttribute().hasName(["unused", "used", "maybe_unused", "cleanup"]) + } +} + +/* Configuration to use the `DedupMacroResults` module to reduce alert noise */ +module UnusedObjectDefinitionDedupeConfig implements + DeduplicateMacroConfigSig +{ + string describe(UnusedObjectDefinition def) { result = def.getName() } +} + +import DeduplicateMacroResults as DeduplicateUnusedMacroObjects + +/** + * A macro invocation that only defines one unused variable. + * + * These are reported at the invocation site when the variable is unused. + */ +class MacroExpansionWithOnlyUnusedObjectDefinition extends MacroInvocation { + UnusedObjectDefinition unusedObject; + + MacroExpansionWithOnlyUnusedObjectDefinition() { + exists(DeclStmt stmt, Declaration decl | + stmt = getStmt() and + count(getStmt()) = 1 and + count(stmt.getADeclaration()) = 1 and + decl = stmt.getADeclaration() and + count(decl.getADeclarationEntry()) = 1 and + unusedObject = decl.getADeclarationEntry() + ) and + not exists(this.getParentInvocation()) + } + + UnusedObjectDefinition getUnusedObject() { result = unusedObject } +} + +/** + * An object definition which is not from a macro, and for which all copies are unused. + * + * Extends the `HoldForAllCopies::LogicalResultElement` class, because these dead objects are often + * duplicated across defines and sometimes aren't marked used due to extractor bugs. + */ +class SimpleDeadObjectDefinition extends HoldsForAllCopies::LogicalResultElement +{ + SimpleDeadObjectDefinition() { not getAnElementInstance().isInMacroExpansion() } + + string getName() { result = getAnElementInstance().getName() } +} + +/* Make a type for reporting these two results in one query */ +newtype TReportDeadObjectAtDefinition = + TSimpleDeadObjectDefinition(SimpleDeadObjectDefinition def) or + TMacroExpansionWithOnlyUnusedObject(MacroExpansionWithOnlyUnusedObjectDefinition def) + +/** + * Class to report simple dead object definitions, and dead objects from macros that do nothing but + * define an object. + * + * To report all cases, make sure to also use the `DeduplicateUnusedMacroObjects::Report` module. + * + * To report these cases, use the methods: + * - `getMessage()` + * - `getPrimaryElement()`, + * - `getOptionalPlaceholderLocation()` + * - `getOptionalPlaceholderMessage()` + */ +class ReportDeadObjectAtDefinition extends TReportDeadObjectAtDefinition { + string toString() { result = getMessage() } + + string getMessage() { + exists(MacroExpansionWithOnlyUnusedObjectDefinition def | + this = TMacroExpansionWithOnlyUnusedObject(def) and + result = "Unused object definition '" + def.getUnusedObject().getName() + "' from macro '$@'." + ) + or + exists(SimpleDeadObjectDefinition def | + this = TSimpleDeadObjectDefinition(def) and + result = "Unused object definition '" + def.getName() + "'." + ) + } + + predicate hasAttrUnused() { + exists(MacroExpansionWithOnlyUnusedObjectDefinition def | + this = TMacroExpansionWithOnlyUnusedObject(def) and + def.getUnusedObject().hasAttrUnused() + ) + or + exists(SimpleDeadObjectDefinition def | + this = TSimpleDeadObjectDefinition(def) and + def.getAnElementInstance().hasAttrUnused() + ) + } + + Element getPrimaryElement() { + this = TMacroExpansionWithOnlyUnusedObject(result) + or + exists(SimpleDeadObjectDefinition def | + this = TSimpleDeadObjectDefinition(def) and + result = def.getAnElementInstance() + ) + } + + Location getOptionalPlaceholderLocation() { + exists(MacroExpansionWithOnlyUnusedObjectDefinition def | + this = TMacroExpansionWithOnlyUnusedObject(def) and + result = def.getMacro().getLocation() + ) + or + exists(SimpleDeadObjectDefinition def | + this = TSimpleDeadObjectDefinition(def) and + result = def.getAnElementInstance().getLocation() + ) + } + + string getOptionalPlaceholderMessage() { + exists(MacroExpansionWithOnlyUnusedObjectDefinition def | + this = TMacroExpansionWithOnlyUnusedObject(def) and + result = def.getMacroName() + ) + or + this = TSimpleDeadObjectDefinition(_) and + result = "" + } +} + +/* Module config to use the `DeduplicateUnusedMacroObjects::Report` module */ +module ReportDeadObjectInMacroConfig implements MacroReportConfigSig { + bindingset[description] + string getMessageSameResultInAllExpansions(Macro m, string description) { + result = "Macro '" + m.getName() + "' defines unused object '" + description + "'." + } + + string getMessageVariedResultInAllExpansions(Macro m) { + result = "Macro '" + m.getName() + "' defines unused object of varied names, for example, '$@'." + } + + string getMessageResultInIsolatedExpansion(UnusedObjectDefinition unused) { + result = "Invocation of macro '$@' defines unused object '" + unused.getName() + "'." + } +} + +/* The object to report in queries of dead objects used in macros */ +class ReportDeadObjectInMacro extends DeduplicateUnusedMacroObjects::Report::ReportResultInMacro +{ + ReportDeadObjectInMacro() { + // `MacroExpansionWithOnlyUnusedObjectDefinition` is reported by class `ReportDeadObjectAtDefinition` + not getAResultMacroExpansion() instanceof MacroExpansionWithOnlyUnusedObjectDefinition + } + + predicate hasAttrUnused() { getAResultMacroExpansion().getResultElement().hasAttrUnused() } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll new file mode 100644 index 0000000000..8f8edc03fa --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll @@ -0,0 +1,78 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype DeadCode2Query = + TUnusedObjectDefinitionQuery() or + TUnusedObjectDefinitionInMacroQuery() or + TUnusedObjectDefinitionStrictQuery() or + TUnusedObjectDefinitionInMacroStrictQuery() + +predicate isDeadCode2QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `unusedObjectDefinition` query + DeadCode2Package::unusedObjectDefinitionQuery() and + queryId = + // `@id` for the `unusedObjectDefinition` query + "c/misra/unused-object-definition" and + ruleId = "RULE-2-8" and + category = "advisory" + or + query = + // `Query` instance for the `unusedObjectDefinitionInMacro` query + DeadCode2Package::unusedObjectDefinitionInMacroQuery() and + queryId = + // `@id` for the `unusedObjectDefinitionInMacro` query + "c/misra/unused-object-definition-in-macro" and + ruleId = "RULE-2-8" and + category = "advisory" + or + query = + // `Query` instance for the `unusedObjectDefinitionStrict` query + DeadCode2Package::unusedObjectDefinitionStrictQuery() and + queryId = + // `@id` for the `unusedObjectDefinitionStrict` query + "c/misra/unused-object-definition-strict" and + ruleId = "RULE-2-8" and + category = "advisory" + or + query = + // `Query` instance for the `unusedObjectDefinitionInMacroStrict` query + DeadCode2Package::unusedObjectDefinitionInMacroStrictQuery() and + queryId = + // `@id` for the `unusedObjectDefinitionInMacroStrict` query + "c/misra/unused-object-definition-in-macro-strict" and + ruleId = "RULE-2-8" and + category = "advisory" +} + +module DeadCode2Package { + Query unusedObjectDefinitionQuery() { + //autogenerate `Query` type + result = + // `Query` type for `unusedObjectDefinition` query + TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionQuery())) + } + + Query unusedObjectDefinitionInMacroQuery() { + //autogenerate `Query` type + result = + // `Query` type for `unusedObjectDefinitionInMacro` query + TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionInMacroQuery())) + } + + Query unusedObjectDefinitionStrictQuery() { + //autogenerate `Query` type + result = + // `Query` type for `unusedObjectDefinitionStrict` query + TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionStrictQuery())) + } + + Query unusedObjectDefinitionInMacroStrictQuery() { + //autogenerate `Query` type + result = + // `Query` type for `unusedObjectDefinitionInMacroStrict` query + TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionInMacroStrictQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index 3833533d50..75aad6f02c 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -19,6 +19,7 @@ import Contracts5 import Contracts6 import Contracts7 import DeadCode +import DeadCode2 import Declarations1 import Declarations2 import Declarations3 @@ -95,6 +96,7 @@ newtype TCQuery = TContracts6PackageQuery(Contracts6Query q) or TContracts7PackageQuery(Contracts7Query q) or TDeadCodePackageQuery(DeadCodeQuery q) or + TDeadCode2PackageQuery(DeadCode2Query q) or TDeclarations1PackageQuery(Declarations1Query q) or TDeclarations2PackageQuery(Declarations2Query q) or TDeclarations3PackageQuery(Declarations3Query q) or @@ -171,6 +173,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isContracts6QueryMetadata(query, queryId, ruleId, category) or isContracts7QueryMetadata(query, queryId, ruleId, category) or isDeadCodeQueryMetadata(query, queryId, ruleId, category) or + isDeadCode2QueryMetadata(query, queryId, ruleId, category) or isDeclarations1QueryMetadata(query, queryId, ruleId, category) or isDeclarations2QueryMetadata(query, queryId, ruleId, category) or isDeclarations3QueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected new file mode 100644 index 0000000000..eb55b83924 --- /dev/null +++ b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected @@ -0,0 +1,6 @@ +| deduplicatemacroresults.cpp:10:1:10:34 | SOMETIMES_HAS_RESULTS1(type,name) | Invocation of macro $@ has findme var 'g3'. | deduplicatemacroresults.cpp:6:1:6:52 | deduplicatemacroresults.cpp:6:1:6:52 | SOMETIMES_HAS_RESULTS1 | +| deduplicatemacroresults.cpp:13:1:13:34 | SOMETIMES_HAS_RESULTS2(type,name) | Invocation of macro $@ has findme var 'g5'. | deduplicatemacroresults.cpp:7:1:7:53 | deduplicatemacroresults.cpp:7:1:7:53 | SOMETIMES_HAS_RESULTS2 | +| deduplicatemacroresults.cpp:15:1:15:50 | #define ALWAYS_HAS_SAME_RESULT() extern findme g6; | Macro ALWAYS_HAS_SAME_RESULT always has findme var named g6 | deduplicatemacroresults.cpp:15:1:15:50 | deduplicatemacroresults.cpp:15:1:15:50 | (ignored) | +| deduplicatemacroresults.cpp:21:1:21:70 | #define ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(name) extern findme name; | Macro ALWAYS_HAS_RESULT_VARIED_DESCRIPTION always has findme var, for example '$@'. | deduplicatemacroresults.cpp:23:38:23:39 | deduplicatemacroresults.cpp:23:38:23:39 | g7 | +| deduplicatemacroresults.cpp:30:1:31:50 | #define OUTER_ALWAYS_HAS_SAME_RESULT() extern INNER_SOMETIMES_HAS_RESULTS(findme, g10); | Macro OUTER_ALWAYS_HAS_SAME_RESULT always has findme var named g10 | deduplicatemacroresults.cpp:30:1:31:50 | deduplicatemacroresults.cpp:30:1:31:50 | (ignored) | +| deduplicatemacroresults.cpp:37:1:38:52 | #define OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(name) INNER_SOMETIMES_HAS_RESULTS(findme, name ## suffix); | Macro OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION always has findme var, for example '$@'. | deduplicatemacroresults.cpp:40:44:40:47 | deduplicatemacroresults.cpp:40:44:40:47 | g11suffix | diff --git a/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql new file mode 100644 index 0000000000..cd999d72c9 --- /dev/null +++ b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql @@ -0,0 +1,32 @@ +import cpp +import codingstandards.cpp.alertreporting.DeduplicateMacroResults + +class FindMe extends VariableDeclarationEntry { + FindMe() { getType().toString() = "findme" } +} + +module FindMeDedupeConfig implements DeduplicateMacroConfigSig { + string describe(FindMe def) { result = def.getName() } +} + +module FindMeReportConfig implements MacroReportConfigSig { + bindingset[description] + string getMessageSameResultInAllExpansions(Macro m, string description) { + result = "Macro " + m.getName() + " always has findme var named " + description + } + + string getMessageVariedResultInAllExpansions(Macro m) { + result = "Macro " + m.getName() + " always has findme var, for example '$@'." + } + + string getMessageResultInIsolatedExpansion(FindMe f) { + result = "Invocation of macro $@ has findme var '" + f.getName() + "'." + } +} + +import DeduplicateMacroResults +import DeduplicateMacroResults::Report + +from ReportResultInMacro report +select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), + report.getOptionalPlaceholderMessage() diff --git a/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp b/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp new file mode 100644 index 0000000000..3c5d8bca5b --- /dev/null +++ b/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp @@ -0,0 +1,53 @@ +typedef struct { +} findme; + +findme g1; // ignore -- not in a macro + +#define SOMETIMES_HAS_RESULTS1(type, name) type name // ignore +#define SOMETIMES_HAS_RESULTS2(type, name) type name; // ignore + +SOMETIMES_HAS_RESULTS1(int, g2); // ignore +SOMETIMES_HAS_RESULTS1(findme, g3); // RESULT + +SOMETIMES_HAS_RESULTS2(int, g4) // ignore +SOMETIMES_HAS_RESULTS2(findme, g5) // RESULT + +#define ALWAYS_HAS_SAME_RESULT() extern findme g6; // RESULT + +ALWAYS_HAS_SAME_RESULT() // ignore +ALWAYS_HAS_SAME_RESULT() // ignore +ALWAYS_HAS_SAME_RESULT() // ignore + +#define ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(name) extern findme name; // RESULT + +ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g7) // ignore +ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g8) // ignore +ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g9) // ignore +ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g9) // ignore +ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g9) // ignore + +#define INNER_SOMETIMES_HAS_RESULTS(type, name) type name; // ignore +#define OUTER_ALWAYS_HAS_SAME_RESULT() \ + extern INNER_SOMETIMES_HAS_RESULTS(findme, g10); // RESULT + +OUTER_ALWAYS_HAS_SAME_RESULT() // ignore +OUTER_ALWAYS_HAS_SAME_RESULT() // ignore + +// 'name ## suffix' required to work around extractor bug. +#define OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(name) \ + INNER_SOMETIMES_HAS_RESULTS(findme, name##suffix); // RESULT + +OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g11) // ignore +OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g12) // ignore + +#define OUTER_OUTER_ALWAYS_HAS_SAME_RESULT() \ + OUTER_ALWAYS_HAS_SAME_RESULT(); // ignore +OUTER_OUTER_ALWAYS_HAS_SAME_RESULT() // ignore +OUTER_OUTER_ALWAYS_HAS_SAME_RESULT() // ignore + +// 'name ## suffix' required to work around extractor bug. +#define OUTER_OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(name) \ + OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(name##suffix); // ignore + +OUTER_OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g13) // ignore +OUTER_OUTER_ALWAYS_HAS_RESULT_VARIED_DESCRIPTION(g14) // ignore \ No newline at end of file diff --git a/rule_packages/c/DeadCode2.json b/rule_packages/c/DeadCode2.json new file mode 100644 index 0000000000..da114a2349 --- /dev/null +++ b/rule_packages/c/DeadCode2.json @@ -0,0 +1,66 @@ +{ + "MISRA-C-2012": { + "RULE-2-8": { + "properties": { + "obligation": "advisory" + }, + "queries": [ + { + "description": "Object definitions which are unused should be removed.", + "kind": "problem", + "name": "A project should not contain unused object definitions", + "precision": "very-high", + "severity": "recommendation", + "short_name": "UnusedObjectDefinition", + "tags": [ + "maintainability", + "performance", + "external/misra/c/2012/amendment4" + ] + }, + { + "description": "Macros should not have unused object definitions.", + "kind": "problem", + "name": "Project macros should not include unused object definitions", + "precision": "very-high", + "severity": "recommendation", + "short_name": "UnusedObjectDefinitionInMacro", + "tags": [ + "maintainability", + "performance", + "external/misra/c/2012/amendment4" + ] + }, + { + "description": "A strict query which reports all unused object definitions with '__attribute__((unused))'.", + "kind": "problem", + "name": "A project should not contain '__attribute__((unused))' object definitions", + "precision": "very-high", + "severity": "recommendation", + "short_name": "UnusedObjectDefinitionStrict", + "tags": [ + "maintainability", + "performance", + "external/misra/c/2012/amendment4", + "external/misra/c/strict" + ] + }, + { + "description": "A strict query which reports all unused object definitions in macros with '__attribute__((unused))'.", + "kind": "problem", + "name": "Project macros should not include '__attribute__((unused))' object definitions", + "precision": "very-high", + "severity": "recommendation", + "short_name": "UnusedObjectDefinitionInMacroStrict", + "tags": [ + "maintainability", + "performance", + "external/misra/c/2012/amendment4", + "external/misra/c/strict" + ] + } + ], + "title": "A project should not contain unused object definitions" + } + } +} \ No newline at end of file From ff562f9e93d803242cc0a8413277daad64f3d376 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 24 Oct 2024 23:31:58 -0700 Subject: [PATCH 2/8] Fix strict misra tag in rules schema --- schemas/rule-package.schema.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/schemas/rule-package.schema.json b/schemas/rule-package.schema.json index b27815163e..b27136634f 100644 --- a/schemas/rule-package.schema.json +++ b/schemas/rule-package.schema.json @@ -342,7 +342,8 @@ "external/misra/c/2012/third-edition-first-revision", "external/misra/c/2012/amendment2", "external/misra/c/2012/amendment3", - "external/misra/c/2012/amendment4" + "external/misra/c/2012/amendment4", + "external/misra/c/strict" ] }, "minLength": 1 From 125650c582ec38fb4785940444fdf6fe927c4efd Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Mon, 9 Dec 2024 16:37:57 -0800 Subject: [PATCH 3/8] Address review feedback --- c/misra/src/codeql-suites/misra-c-default.qls | 1 + c/misra/src/codeql-suites/misra-c-strict.qls | 8 ++++ .../RULE-2-8/UnusedObjectDefinition.expected | 2 + c/misra/test/rules/RULE-2-8/test.c | 18 +++++++++ .../cpp/deadcode/UnusedObjects.qll | 10 +++-- .../cpp/deadcode/UnusedVariables.qll | 39 ++++++++++++++----- docs/development_handbook.md | 2 + docs/user_manual.md | 13 ++++++- rule_packages/c/DeadCode2.json | 5 ++- 9 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 c/misra/src/codeql-suites/misra-c-strict.qls diff --git a/c/misra/src/codeql-suites/misra-c-default.qls b/c/misra/src/codeql-suites/misra-c-default.qls index 343379a2b3..f72a63ba49 100644 --- a/c/misra/src/codeql-suites/misra-c-default.qls +++ b/c/misra/src/codeql-suites/misra-c-default.qls @@ -7,4 +7,5 @@ - exclude: tags contain: - external/misra/audit + - external/misra/strict - external/misra/default-disabled diff --git a/c/misra/src/codeql-suites/misra-c-strict.qls b/c/misra/src/codeql-suites/misra-c-strict.qls new file mode 100644 index 0000000000..6fb642424c --- /dev/null +++ b/c/misra/src/codeql-suites/misra-c-strict.qls @@ -0,0 +1,8 @@ +- description: MISRA C 2012 (Strict) +- qlpack: codeql/misra-c-coding-standards +- include: + kind: + - problem + - path-problem + tags contain: + - external/misra/strict diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected index fc6f320539..ce7e198122 100644 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected @@ -6,3 +6,5 @@ | test.c:45:9:45:10 | definition of g6 | Unused object definition 'g6'. | test.c:45:9:45:10 | test.c:45:9:45:10 | | | test.c:51:5:51:6 | definition of g7 | Unused object definition 'g7'. | test.c:51:5:51:6 | test.c:51:5:51:6 | | | test.c:64:3:64:18 | ONLY_DEF_VAR(x) | Unused object definition 'l2' from macro '$@'. | test.c:60:1:60:34 | test.c:60:1:60:34 | ONLY_DEF_VAR | +| test.c:117:11:117:13 | definition of g10 | Unused object definition 'g10'. | test.c:117:11:117:13 | test.c:117:11:117:13 | | +| test.c:122:13:122:14 | definition of l2 | Unused object definition 'l2'. | test.c:122:13:122:14 | test.c:122:13:122:14 | | diff --git a/c/misra/test/rules/RULE-2-8/test.c b/c/misra/test/rules/RULE-2-8/test.c index 21a2479163..cffb4f2e33 100644 --- a/c/misra/test/rules/RULE-2-8/test.c +++ b/c/misra/test/rules/RULE-2-8/test.c @@ -111,3 +111,21 @@ void f8() { void f9() { DEF_ATTR_UNUSED_INNER_VAR(); // COMPLIANT } + +// Const variable tests: +const int g9 = 1; // COMPLIANT +const int g10 = 1; // NON-COMPLIANT + +void f10() { + g9; + const int l1 = 1; // COMPLIANT + const int l2 = 1; // NON-COMPLIANT + l1; +} + +// Side effects should not disable this rule: +void f11() { + int l1 = 1; // COMPLIANT + int l2 = l1++; // COMPLIANT + l2; +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll index 70944dfad4..96dcc8d315 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll @@ -1,4 +1,5 @@ import cpp +import codingstandards.cpp.deadcode.UnusedVariables import codingstandards.cpp.alertreporting.HoldsForAllCopies import codingstandards.cpp.alertreporting.DeduplicateMacroResults @@ -15,10 +16,13 @@ import codingstandards.cpp.alertreporting.DeduplicateMacroResults */ class UnusedObjectDefinition extends VariableDeclarationEntry { UnusedObjectDefinition() { + ( + getVariable() instanceof BasePotentiallyUnusedLocalVariable + or + getVariable() instanceof BasePotentiallyUnusedGlobalOrNamespaceVariable + ) and not exists(VariableAccess access | access.getTarget() = getVariable()) and - getVariable().getDefinition() = this and - not this instanceof ParameterDeclarationEntry and - not getVariable() instanceof MemberVariable + getVariable().getDefinition() = this } /* Dead objects with these attributes are reported in the "strict" queries. */ diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index 912d2babcd..b896fb6f9e 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -36,11 +36,11 @@ predicate declarationHasSideEffects(Variable v) { v.getType() instanceof TemplateDependentType } -/** A `LocalVariable` which is a candidate for being unused. */ -class PotentiallyUnusedLocalVariable extends LocalVariable { - PotentiallyUnusedLocalVariable() { - // Ignore variables declared in macro expansions - not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion()) and +/** + * A `LocalVariable` which is a candidate for being unused, and may or may not be defined in a macro. + */ +class BasePotentiallyUnusedLocalVariable extends LocalVariable { + BasePotentiallyUnusedLocalVariable() { // Ignore variables where initializing the variable has side effects not declarationHasSideEffects(this) and // TODO non POD types with initializers? Also, do something different with templates? exists(Function f | f = getFunction() | @@ -56,6 +56,16 @@ class PotentiallyUnusedLocalVariable extends LocalVariable { } } +/** + * A `LocalVariable` which is a candidate for being unused, and not defined in a macro. + */ +class PotentiallyUnusedLocalVariable extends BasePotentiallyUnusedLocalVariable { + PotentiallyUnusedLocalVariable() { + // Ignore variables declared in macro expansions + not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion()) + } +} + /** Holds if `mf` is "defined" in this database. */ predicate isDefined(MemberFunction mf) { exists(MemberFunction definedMemberFunction | @@ -105,13 +115,11 @@ class PotentiallyUnusedMemberVariable extends MemberVariable { } } -/** A `GlobalOrNamespaceVariable` which is potentially unused. */ -class PotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVariable { - PotentiallyUnusedGlobalOrNamespaceVariable() { +/** A `GlobalOrNamespaceVariable` which is potentially unused and may or may not be defined in a macro */ +class BasePotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVariable { + BasePotentiallyUnusedGlobalOrNamespaceVariable() { // A non-defined variable may never be used hasDefinition() and - // Not declared in a macro expansion - not isInMacroExpansion() and // No side-effects from declaration not declarationHasSideEffects(this) and // exclude uninstantiated template members @@ -121,6 +129,17 @@ class PotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVariab } } +/** + * A `GlobalOrNamespaceVariable` which is potentially unused, and is not defined in a macro. +*/ +class PotentiallyUnusedGlobalOrNamespaceVariable extends BasePotentiallyUnusedGlobalOrNamespaceVariable +{ + PotentiallyUnusedGlobalOrNamespaceVariable() { + // Not declared in a macro expansion + not isInMacroExpansion() + } +} + predicate isUnused(Variable variable) { not exists(variable.getAnAccess()) and variable.getInitializer().fromSource() diff --git a/docs/development_handbook.md b/docs/development_handbook.md index dc50bf59ff..97c615ba2e 100644 --- a/docs/development_handbook.md +++ b/docs/development_handbook.md @@ -51,6 +51,8 @@ Each coding standard consists of a list of "guidelines", however not all the gui For some of the rules which are not amenable to static analysis, we may opt to provide a query which aids with "auditing" the rules. For example, AUTOSAR includes a rule (A10-0-1) "Public inheritance shall be used to implement 'is-a' relationship.". This is not directly amenable to static analysis, because it requires external context around the concept being modeled. However, we can provide an "audit" rule which reports all the public and private inheritance relationships in the program, so they can be manually verified. +For other rules, there may be means of indicating that a contravention is intentional, and where requiring a _devation report_ may be extra burdensome on developers and require double-entry. These results should be reported under a "strict" query. For instance, `RULE-2-8` "A project should not contain unused object definitions," where adding `__attribute__((unused))` may be preferable in order to suppress compiler warnings (which _deviation reports_ do not do) and are highly indicative of an intentional contravention by a developer. + For each rule which will be implemented with a query we have assigned a "rule package". Rule packages represent sets of rules, possibly across standards, that will be implemented together. Examples of rule packages include "Exceptions", "Naming", "Pointers" and so forth. By implementing queries for related rules together, we intend to maximize the amount of code shared between queries, and to ensure query developers can gain a deep understanding of that specific topic. The canonical list of rules, with implementation categorization and assigned rule packages, are stored in this repository in the `rules.csv` file. diff --git a/docs/user_manual.md b/docs/user_manual.md index db0f836339..1ddf68870e 100644 --- a/docs/user_manual.md +++ b/docs/user_manual.md @@ -71,6 +71,7 @@ For each rule we therefore identify whether it is supportable or not. Furthermor - **Automated** - the queries for the rule find contraventions directly. - **Audit only** - the queries for the rule does not find contraventions directly, but instead report a list of _candidates_ that can be used as input into a manual audit. For example, `A10-0-1` (_Public inheritance shall be used to implement 'is-a' relationship_) is not directly amenable to static analysis, but CodeQL can be used to produce a list of all the locations that use public inheritance so they can be manually reviewed. +- **Strict only** - the queries for the rule find contraventions directly, but find results which are strongly indicated to be intentional, and where adding a _deviation report_ may be extra burden on developers. For example, in `RULE-2-8` (_A project should not contain unused object definitions_), declaring objects with `__attribute__((unused))` may be preferable to a _deviation report_, which will not suppress relevant compiler warnings, and therefore would otherwise require developer double-entry. Each supported rule is implemented as one or more CodeQL queries, with each query covering an aspect of the rule. In many coding standards, the rules cover non-trivial semantic properties of the codebase under analysis. @@ -214,14 +215,22 @@ The following flags may be passed to the `database analyze` command to adjust th The output of this command will be a [SARIF file](https://sarifweb.azurewebsites.net/) called `.sarif`. -#### Running the analysis for audit level queries +#### Running the analysis for strict and/or audit level queries -Optionally, you may want to run the "audit" level queries. These queries produce lists of results that do not directly highlight contraventions of the rule. Instead, they identify locations in the code that can be manually audited to verify the absence of problems for that particular rule. +Optionally, you may want to run the "strict" or "audit" level queries. + +Audit queries produce lists of results that do not directly highlight contraventions of the rule. Instead, they identify locations in the code that can be manually audited to verify the absence of problems for that particular rule. ```bash codeql database analyze --format=sarifv2.1.0 --output=.sarif path/to/ path/to/codeql-coding-standards/cpp//src/codeql-suites/-audit.qls... ``` +Strict queries identify contraventions in the code that strongly suggest they are deliberate, and where adding an explicit _deviation report_ may be extra burden on developers. + +```bash +codeql database analyze --format=sarifv2.1.0 --output=.sarif path/to/ path/to/codeql-coding-standards/cpp//src/codeql-suites/-strict.qls... +``` + For each Coding Standard you want to run, add a trailing entry in the following format: `path/to/codeql-coding-standards/cpp//src/codeql-suites/-default.qls`. #### Producing an analysis report diff --git a/rule_packages/c/DeadCode2.json b/rule_packages/c/DeadCode2.json index da114a2349..b897f595e6 100644 --- a/rule_packages/c/DeadCode2.json +++ b/rule_packages/c/DeadCode2.json @@ -60,7 +60,10 @@ ] } ], - "title": "A project should not contain unused object definitions" + "title": "A project should not contain unused object definitions", + "implementation_scope": { + "description": "Unused object definitions marked with `__attribute__((unused))` (and `used`, `maybe_used`, `cleanup`) are separately reported under the 'strict' query suite. This is because these attributes strongly indicate the contravention is intentional, and a deviation report alone will not suppress compiler warnings." + } } } } \ No newline at end of file From 9492933c0ac32ac1a98b2bcb51e8f985f5ce5d3b Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Tue, 10 Dec 2024 08:25:01 -0800 Subject: [PATCH 4/8] Format --- cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll index b896fb6f9e..a7accd5252 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll @@ -131,7 +131,7 @@ class BasePotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVa /** * A `GlobalOrNamespaceVariable` which is potentially unused, and is not defined in a macro. -*/ + */ class PotentiallyUnusedGlobalOrNamespaceVariable extends BasePotentiallyUnusedGlobalOrNamespaceVariable { PotentiallyUnusedGlobalOrNamespaceVariable() { From 8c31c8c296b5992b931fe1cbc8b338b89e8dc048 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Tue, 10 Dec 2024 08:26:37 -0800 Subject: [PATCH 5/8] format c --- c/misra/test/rules/RULE-2-8/test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/c/misra/test/rules/RULE-2-8/test.c b/c/misra/test/rules/RULE-2-8/test.c index cffb4f2e33..ef40dfb2a4 100644 --- a/c/misra/test/rules/RULE-2-8/test.c +++ b/c/misra/test/rules/RULE-2-8/test.c @@ -113,7 +113,7 @@ void f9() { } // Const variable tests: -const int g9 = 1; // COMPLIANT +const int g9 = 1; // COMPLIANT const int g10 = 1; // NON-COMPLIANT void f10() { @@ -125,7 +125,7 @@ void f10() { // Side effects should not disable this rule: void f11() { - int l1 = 1; // COMPLIANT + int l1 = 1; // COMPLIANT int l2 = l1++; // COMPLIANT l2; } \ No newline at end of file From a75dc0af9db6f80aaff1ca18af5839912c9edc8c Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Wed, 15 Jan 2025 20:01:39 -0800 Subject: [PATCH 6/8] Address feedback --- .../rules/RULE-2-8/UnusedObjectDefinition.ql | 2 +- .../RULE-2-8/UnusedObjectDefinitionInMacro.ql | 24 ---- .../UnusedObjectDefinitionInMacroStrict.ql | 27 ---- .../RULE-2-8/UnusedObjectDefinitionStrict.ql | 2 +- .../RULE-2-8/UnusedObjectDefinition.expected | 22 +-- .../UnusedObjectDefinitionInMacro.expected | 2 - .../UnusedObjectDefinitionInMacro.qlref | 1 - ...usedObjectDefinitionInMacroStrict.expected | 2 - .../UnusedObjectDefinitionInMacroStrict.qlref | 1 - .../UnusedObjectDefinitionStrict.expected | 6 +- c/misra/test/rules/RULE-2-8/test.c | 2 + .../DeduplicateMacroResults.qll | 55 ++++++-- .../cpp/deadcode/UnusedObjects.qll | 129 ++---------------- .../cpp/exclusions/c/DeadCode2.qll | 36 +---- .../DeduplicateMacroResults.expected | 1 + .../alertreporting/DeduplicateMacroResults.ql | 7 +- .../deduplicatemacroresults.cpp | 2 +- docs/development_handbook.md | 1 + docs/user_manual.md | 3 +- rule_packages/c/DeadCode2.json | 27 ---- 20 files changed, 84 insertions(+), 268 deletions(-) delete mode 100644 c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql delete mode 100644 c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql delete mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected delete mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref delete mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected delete mode 100644 c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql index 420733d4ac..2230a74592 100644 --- a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql +++ b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql @@ -16,7 +16,7 @@ import cpp import codingstandards.c.misra import codingstandards.cpp.deadcode.UnusedObjects -from ReportDeadObjectAtDefinition report +from ReportDeadObject report where not isExcluded(report.getPrimaryElement(), DeadCode2Package::unusedObjectDefinitionQuery()) and not report.hasAttrUnused() diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql deleted file mode 100644 index d5c339c157..0000000000 --- a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql +++ /dev/null @@ -1,24 +0,0 @@ -/** - * @id c/misra/unused-object-definition-in-macro - * @name RULE-2-8: Project macros should not include unused object definitions - * @description Macros should not have unused object definitions. - * @kind problem - * @precision very-high - * @problem.severity recommendation - * @tags external/misra/id/rule-2-8 - * maintainability - * performance - * external/misra/c/2012/amendment4 - * external/misra/obligation/advisory - */ - -import cpp -import codingstandards.c.misra -import codingstandards.cpp.deadcode.UnusedObjects - -from ReportDeadObjectInMacro report -where - not isExcluded(report.getPrimaryElement(), DeadCode2Package::unusedObjectDefinitionInMacroQuery()) and - not report.hasAttrUnused() -select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), - report.getOptionalPlaceholderMessage() diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql deleted file mode 100644 index 7eead60424..0000000000 --- a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql +++ /dev/null @@ -1,27 +0,0 @@ -/** - * @id c/misra/unused-object-definition-in-macro-strict - * @name RULE-2-8: Project macros should not include '__attribute__((unused))' object definitions - * @description A strict query which reports all unused object definitions in macros with - * '__attribute__((unused))'. - * @kind problem - * @precision very-high - * @problem.severity recommendation - * @tags external/misra/id/rule-2-8 - * maintainability - * performance - * external/misra/c/2012/amendment4 - * external/misra/c/strict - * external/misra/obligation/advisory - */ - -import cpp -import codingstandards.c.misra -import codingstandards.cpp.deadcode.UnusedObjects - -from ReportDeadObjectInMacro report -where - not isExcluded(report.getPrimaryElement(), - DeadCode2Package::unusedObjectDefinitionInMacroStrictQuery()) and - report.hasAttrUnused() -select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), - report.getOptionalPlaceholderMessage() diff --git a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql index ad92c79481..cc117763ee 100644 --- a/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql +++ b/c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql @@ -18,7 +18,7 @@ import cpp import codingstandards.c.misra import codingstandards.cpp.deadcode.UnusedObjects -from ReportDeadObjectAtDefinition report +from ReportDeadObject report where not isExcluded(report.getPrimaryElement(), DeadCode2Package::unusedObjectDefinitionStrictQuery()) and report.hasAttrUnused() diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected index ce7e198122..731aebb1be 100644 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected @@ -1,10 +1,12 @@ -| test.c:6:5:6:6 | definition of g2 | Unused object definition 'g2'. | test.c:6:5:6:6 | test.c:6:5:6:6 | | -| test.c:9:5:9:6 | definition of g3 | Unused object definition 'g3'. | test.c:9:5:9:6 | test.c:9:5:9:6 | | -| test.c:20:7:20:8 | definition of l2 | Unused object definition 'l2'. | test.c:20:7:20:8 | test.c:20:7:20:8 | | -| test.c:27:7:27:8 | definition of l5 | Unused object definition 'l5'. | test.c:27:7:27:8 | test.c:27:7:27:8 | | -| test.c:37:10:37:11 | definition of g5 | Unused object definition 'g5'. | test.c:37:10:37:11 | test.c:37:10:37:11 | | -| test.c:45:9:45:10 | definition of g6 | Unused object definition 'g6'. | test.c:45:9:45:10 | test.c:45:9:45:10 | | -| test.c:51:5:51:6 | definition of g7 | Unused object definition 'g7'. | test.c:51:5:51:6 | test.c:51:5:51:6 | | -| test.c:64:3:64:18 | ONLY_DEF_VAR(x) | Unused object definition 'l2' from macro '$@'. | test.c:60:1:60:34 | test.c:60:1:60:34 | ONLY_DEF_VAR | -| test.c:117:11:117:13 | definition of g10 | Unused object definition 'g10'. | test.c:117:11:117:13 | test.c:117:11:117:13 | | -| test.c:122:13:122:14 | definition of l2 | Unused object definition 'l2'. | test.c:122:13:122:14 | test.c:122:13:122:14 | | +| test.c:6:5:6:6 | definition of g2 | Unused object 'g2'. | test.c:6:5:6:6 | test.c:6:5:6:6 | (ignored) | +| test.c:9:5:9:6 | definition of g3 | Unused object 'g3'. | test.c:9:5:9:6 | test.c:9:5:9:6 | (ignored) | +| test.c:20:7:20:8 | definition of l2 | Unused object 'l2'. | test.c:20:7:20:8 | test.c:20:7:20:8 | (ignored) | +| test.c:27:7:27:8 | definition of l5 | Unused object 'l5'. | test.c:27:7:27:8 | test.c:27:7:27:8 | (ignored) | +| test.c:37:10:37:11 | definition of g5 | Unused object 'g5'. | test.c:37:10:37:11 | test.c:37:10:37:11 | (ignored) | +| test.c:45:9:45:10 | definition of g6 | Unused object 'g6'. | test.c:45:9:45:10 | test.c:45:9:45:10 | (ignored) | +| test.c:51:5:51:6 | definition of g7 | Unused object 'g7'. | test.c:51:5:51:6 | test.c:51:5:51:6 | (ignored) | +| test.c:64:3:64:18 | ONLY_DEF_VAR(x) | Invocation of macro '$@' defines unused object 'l2'. | test.c:60:1:60:34 | test.c:60:1:60:34 | ONLY_DEF_VAR | +| test.c:68:1:71:5 | #define ALSO_DEF_VAR(x) int x = 0; while (1) ; | Macro 'ALSO_DEF_VAR' defines unused object with an invocation-dependent name, for example, '$@'. | test.c:73:16:73:17 | test.c:73:16:73:17 | l1 | +| test.c:77:1:82:3 | #define DEF_UNUSED_INNER_VAR() { int _v = 0; while (1) ; } | Macro 'DEF_UNUSED_INNER_VAR' defines unused object '_v'. | test.c:77:1:82:3 | test.c:77:1:82:3 | (ignored) | +| test.c:119:11:119:13 | definition of g10 | Unused object 'g10'. | test.c:119:11:119:13 | test.c:119:11:119:13 | (ignored) | +| test.c:124:13:124:14 | definition of l2 | Unused object 'l2'. | test.c:124:13:124:14 | test.c:124:13:124:14 | (ignored) | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected deleted file mode 100644 index c25c136789..0000000000 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.expected +++ /dev/null @@ -1,2 +0,0 @@ -| test.c:68:1:71:5 | #define ALSO_DEF_VAR(x) int x = 0; while (1) ; | Macro 'ALSO_DEF_VAR' defines unused object of varied names, for example, '$@'. | test.c:73:16:73:17 | test.c:73:16:73:17 | l1 | -| test.c:77:1:82:3 | #define DEF_UNUSED_INNER_VAR() { int _v = 0; while (1) ; } | Macro 'DEF_UNUSED_INNER_VAR' defines unused object '_v'. | test.c:77:1:82:3 | test.c:77:1:82:3 | (ignored) | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref deleted file mode 100644 index 057e684fd0..0000000000 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacro.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected deleted file mode 100644 index 2919c65cb7..0000000000 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.expected +++ /dev/null @@ -1,2 +0,0 @@ -| test.c:94:1:97:5 | #define ALSO_DEF_ATTR_UNUSED_VAR(x) __attribute__((unused)) int x = 0; while (1) ; | Macro 'ALSO_DEF_ATTR_UNUSED_VAR' defines unused object of varied names, for example, '$@'. | test.c:99:28:99:29 | test.c:99:28:99:29 | l1 | -| test.c:104:1:109:3 | #define DEF_ATTR_UNUSED_INNER_VAR() { __attribute__((unused)) int _v = 0; while (1) ; } | Macro 'DEF_ATTR_UNUSED_INNER_VAR' defines unused object '_v'. | test.c:104:1:109:3 | test.c:104:1:109:3 | (ignored) | diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref deleted file mode 100644 index f04653dcb6..0000000000 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected index 624368ac54..cf3c0b64e1 100644 --- a/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected +++ b/c/misra/test/rules/RULE-2-8/UnusedObjectDefinitionStrict.expected @@ -1,2 +1,4 @@ -| test.c:87:29:87:30 | definition of g8 | Unused object definition 'g8'. | test.c:87:29:87:30 | test.c:87:29:87:30 | | -| test.c:90:3:90:30 | ONLY_DEF_ATTR_UNUSED_VAR(x) | Unused object definition 'l2' from macro '$@'. | test.c:88:1:88:70 | test.c:88:1:88:70 | ONLY_DEF_ATTR_UNUSED_VAR | +| test.c:87:29:87:30 | definition of g8 | Unused object 'g8'. | test.c:87:29:87:30 | test.c:87:29:87:30 | (ignored) | +| test.c:92:3:92:30 | ONLY_DEF_ATTR_UNUSED_VAR(x) | Invocation of macro '$@' defines unused object 'l2'. | test.c:88:1:88:70 | test.c:88:1:88:70 | ONLY_DEF_ATTR_UNUSED_VAR | +| test.c:96:1:99:5 | #define ALSO_DEF_ATTR_UNUSED_VAR(x) __attribute__((unused)) int x = 0; while (1) ; | Macro 'ALSO_DEF_ATTR_UNUSED_VAR' defines unused object with an invocation-dependent name, for example, '$@'. | test.c:101:28:101:29 | test.c:101:28:101:29 | l1 | +| test.c:106:1:111:3 | #define DEF_ATTR_UNUSED_INNER_VAR() { __attribute__((unused)) int _v = 0; while (1) ; } | Macro 'DEF_ATTR_UNUSED_INNER_VAR' defines unused object '_v'. | test.c:106:1:111:3 | test.c:106:1:111:3 | (ignored) | diff --git a/c/misra/test/rules/RULE-2-8/test.c b/c/misra/test/rules/RULE-2-8/test.c index ef40dfb2a4..e35bf15567 100644 --- a/c/misra/test/rules/RULE-2-8/test.c +++ b/c/misra/test/rules/RULE-2-8/test.c @@ -87,6 +87,8 @@ void f6() { __attribute__((unused)) int g8 = 1; // NON-COMPLIANT #define ONLY_DEF_ATTR_UNUSED_VAR(x) __attribute__((unused)) int x = 0; void f7() { + ONLY_DEF_ATTR_UNUSED_VAR(l1); // COMPLIANT + l1; ONLY_DEF_ATTR_UNUSED_VAR(l2); // NON-COMPLIANT } diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll index 7c4e8ef41d..6462ac855a 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll @@ -29,12 +29,16 @@ signature module MacroReportConfigSig { /* Create a message to describe this macro, using '$@' to describe an example `ResultElement`. */ string getMessageVariedResultInAllExpansions(Macro m); - /* + /** * Create a message to describe this macro expansion which produces a `ResultElement`, using '$@' * to describe the relevant macro. */ - string getMessageResultInIsolatedExpansion(ResultElement element); + + /** + * Create a message to describe a `ResultElement` which is not generated by a macro expansion. + */ + string getMessageNotInMacro(ResultElement element); } /** @@ -88,7 +92,7 @@ signature module MacroReportConfigSig { * ## Generating Report Objects * * This module also can be used to more easily report issues across these cases, by implementing - * `MacroReportConfigSig` and importing `DeduplicateMacroResults::Report::ReportResultInMacro`. + * `MacroReportConfigSig` and importing `DeduplicateMacroResults::Report::ReportResult`. * * ``` * module InvalidFooInMacroReportConfig implements MacroReportConfigSig { @@ -106,11 +110,15 @@ signature module MacroReportConfigSig { * string getMessageResultInIsolatedExpansion(InvalidFoo foo) { * result = "Invocation of macro $@ has invalid foo '" + foo.getName() + "'." * } + * + * string getMessageNotInMacro(ResultElement element) { + * result = "Invalid foo '" + element.getName() + "'." + * } * } * * import DeduplicateFooInMacros::Report as Report; * - * from Report::ReportResultInMacro report + * from Report::ReportResult report * where not excluded(report.getPrimaryElement(), ...) * select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), * report.getOptionalPlaceholderMessage() @@ -234,11 +242,10 @@ module DeduplicateMacroResults< } } - /* + /** * Convenience predicate to know when invalid macro expansions have been reported at their macro * definition. */ - private predicate reported(Macro macro) { macro instanceof PrimaryMacroSameResultElementInAllInvocations or macro instanceof PrimaryMacroDifferentResultElementInAllInvocations @@ -248,7 +255,7 @@ module DeduplicateMacroResults< * A macro invocation for which the target macro does not always produce a `ResultElement`, but * this specific invocation of it does. * - * This is "primary" / most specific macro for these result elements. It will also does not match + * This is the "primary" / most specific macro for these result elements. It also does not match * `MacroInvocation`s inside of a `MacroInvocation` of a `Macro` which always produces a * `ResultElement`, indicating that the real problem lies with that other `Macro` instead of with * this particular invocation. @@ -274,10 +281,15 @@ module DeduplicateMacroResults< * See the doc comment for the `DeduplicateMacroResults` module for more info. */ module Report ReportConfig> { - newtype TReportResultInMacro = + newtype TReportResult = TReportMacroResultWithSameName(PrimaryMacroSameResultElementInAllInvocations def) or TReportMacroResultWithVariedName(PrimaryMacroDifferentResultElementInAllInvocations def) or - TReportIsolatedMacroResult(IsolatedMacroExpansionWithResultElement def) + TReportIsolatedMacroResult(IsolatedMacroExpansionWithResultElement def) or + TReportNotInMacro(ResultElement def) { + not exists (ResultMacroExpansion macroExpansion | + macroExpansion.getResultElement() = def + ) + } /** * An instance of a `ResultElement` to be reported to a user. @@ -291,7 +303,7 @@ module DeduplicateMacroResults< * The values returned by these methods are configured by the `MacroReportConfigSig` * signature parameter. */ - class ReportResultInMacro extends TReportResultInMacro { + class ReportResult extends TReportResult { string toString() { result = getMessage() } string getMessage() { @@ -310,6 +322,11 @@ module DeduplicateMacroResults< this = TReportIsolatedMacroResult(def) and result = ReportConfig::getMessageResultInIsolatedExpansion(def.getResultElement()) ) + or + exists(ResultElement def | + this = TReportNotInMacro(def) and + result = ReportConfig::getMessageNotInMacro(def) + ) } Element getPrimaryElement() { @@ -318,6 +335,8 @@ module DeduplicateMacroResults< this = TReportMacroResultWithVariedName(result) or this = TReportIsolatedMacroResult(result) + or + this = TReportNotInMacro(result) } Location getOptionalPlaceholderLocation() { @@ -335,6 +354,11 @@ module DeduplicateMacroResults< this = TReportIsolatedMacroResult(def) and result = def.getMacro().getLocation() ) + or + exists(ResultElement def | + this = TReportNotInMacro(def) and + result = def.getLocation() + ) } string getOptionalPlaceholderMessage() { @@ -343,7 +367,10 @@ module DeduplicateMacroResults< result = Config::describe(def.getExampleResultElement()) ) or - this = TReportMacroResultWithSameName(_) and + ( + this = TReportMacroResultWithSameName(_) + or this = TReportNotInMacro(_) + ) and result = "(ignored)" or this = TReportIsolatedMacroResult(_) and @@ -374,6 +401,12 @@ module DeduplicateMacroResults< or this = TReportIsolatedMacroResult(result) } + + ResultElement getAResultElement() { + result = getAResultMacroExpansion().getResultElement() + or + this = TReportNotInMacro(result) + } } } } diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll index 96dcc8d315..fc262b5d93 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll @@ -40,141 +40,28 @@ module UnusedObjectDefinitionDedupeConfig implements import DeduplicateMacroResults as DeduplicateUnusedMacroObjects -/** - * A macro invocation that only defines one unused variable. - * - * These are reported at the invocation site when the variable is unused. - */ -class MacroExpansionWithOnlyUnusedObjectDefinition extends MacroInvocation { - UnusedObjectDefinition unusedObject; - - MacroExpansionWithOnlyUnusedObjectDefinition() { - exists(DeclStmt stmt, Declaration decl | - stmt = getStmt() and - count(getStmt()) = 1 and - count(stmt.getADeclaration()) = 1 and - decl = stmt.getADeclaration() and - count(decl.getADeclarationEntry()) = 1 and - unusedObject = decl.getADeclarationEntry() - ) and - not exists(this.getParentInvocation()) - } - - UnusedObjectDefinition getUnusedObject() { result = unusedObject } -} - -/** - * An object definition which is not from a macro, and for which all copies are unused. - * - * Extends the `HoldForAllCopies::LogicalResultElement` class, because these dead objects are often - * duplicated across defines and sometimes aren't marked used due to extractor bugs. - */ -class SimpleDeadObjectDefinition extends HoldsForAllCopies::LogicalResultElement -{ - SimpleDeadObjectDefinition() { not getAnElementInstance().isInMacroExpansion() } - - string getName() { result = getAnElementInstance().getName() } -} - -/* Make a type for reporting these two results in one query */ -newtype TReportDeadObjectAtDefinition = - TSimpleDeadObjectDefinition(SimpleDeadObjectDefinition def) or - TMacroExpansionWithOnlyUnusedObject(MacroExpansionWithOnlyUnusedObjectDefinition def) - -/** - * Class to report simple dead object definitions, and dead objects from macros that do nothing but - * define an object. - * - * To report all cases, make sure to also use the `DeduplicateUnusedMacroObjects::Report` module. - * - * To report these cases, use the methods: - * - `getMessage()` - * - `getPrimaryElement()`, - * - `getOptionalPlaceholderLocation()` - * - `getOptionalPlaceholderMessage()` - */ -class ReportDeadObjectAtDefinition extends TReportDeadObjectAtDefinition { - string toString() { result = getMessage() } - - string getMessage() { - exists(MacroExpansionWithOnlyUnusedObjectDefinition def | - this = TMacroExpansionWithOnlyUnusedObject(def) and - result = "Unused object definition '" + def.getUnusedObject().getName() + "' from macro '$@'." - ) - or - exists(SimpleDeadObjectDefinition def | - this = TSimpleDeadObjectDefinition(def) and - result = "Unused object definition '" + def.getName() + "'." - ) - } - - predicate hasAttrUnused() { - exists(MacroExpansionWithOnlyUnusedObjectDefinition def | - this = TMacroExpansionWithOnlyUnusedObject(def) and - def.getUnusedObject().hasAttrUnused() - ) - or - exists(SimpleDeadObjectDefinition def | - this = TSimpleDeadObjectDefinition(def) and - def.getAnElementInstance().hasAttrUnused() - ) - } - - Element getPrimaryElement() { - this = TMacroExpansionWithOnlyUnusedObject(result) - or - exists(SimpleDeadObjectDefinition def | - this = TSimpleDeadObjectDefinition(def) and - result = def.getAnElementInstance() - ) - } - - Location getOptionalPlaceholderLocation() { - exists(MacroExpansionWithOnlyUnusedObjectDefinition def | - this = TMacroExpansionWithOnlyUnusedObject(def) and - result = def.getMacro().getLocation() - ) - or - exists(SimpleDeadObjectDefinition def | - this = TSimpleDeadObjectDefinition(def) and - result = def.getAnElementInstance().getLocation() - ) - } - - string getOptionalPlaceholderMessage() { - exists(MacroExpansionWithOnlyUnusedObjectDefinition def | - this = TMacroExpansionWithOnlyUnusedObject(def) and - result = def.getMacroName() - ) - or - this = TSimpleDeadObjectDefinition(_) and - result = "" - } -} - /* Module config to use the `DeduplicateUnusedMacroObjects::Report` module */ -module ReportDeadObjectInMacroConfig implements MacroReportConfigSig { +module ReportDeadObjectConfig implements MacroReportConfigSig { bindingset[description] string getMessageSameResultInAllExpansions(Macro m, string description) { result = "Macro '" + m.getName() + "' defines unused object '" + description + "'." } string getMessageVariedResultInAllExpansions(Macro m) { - result = "Macro '" + m.getName() + "' defines unused object of varied names, for example, '$@'." + result = "Macro '" + m.getName() + "' defines unused object with an invocation-dependent name, for example, '$@'." } string getMessageResultInIsolatedExpansion(UnusedObjectDefinition unused) { result = "Invocation of macro '$@' defines unused object '" + unused.getName() + "'." } + + string getMessageNotInMacro(UnusedObjectDefinition unused) { + result = "Unused object '" + unused.getName() + "'." + } } /* The object to report in queries of dead objects used in macros */ -class ReportDeadObjectInMacro extends DeduplicateUnusedMacroObjects::Report::ReportResultInMacro +class ReportDeadObject extends DeduplicateUnusedMacroObjects::Report::ReportResult { - ReportDeadObjectInMacro() { - // `MacroExpansionWithOnlyUnusedObjectDefinition` is reported by class `ReportDeadObjectAtDefinition` - not getAResultMacroExpansion() instanceof MacroExpansionWithOnlyUnusedObjectDefinition - } - - predicate hasAttrUnused() { getAResultMacroExpansion().getResultElement().hasAttrUnused() } + predicate hasAttrUnused() { getAResultElement().hasAttrUnused() } } diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll index 8f8edc03fa..611062a5ac 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/DeadCode2.qll @@ -5,9 +5,7 @@ import codingstandards.cpp.exclusions.RuleMetadata newtype DeadCode2Query = TUnusedObjectDefinitionQuery() or - TUnusedObjectDefinitionInMacroQuery() or - TUnusedObjectDefinitionStrictQuery() or - TUnusedObjectDefinitionInMacroStrictQuery() + TUnusedObjectDefinitionStrictQuery() predicate isDeadCode2QueryMetadata(Query query, string queryId, string ruleId, string category) { query = @@ -19,15 +17,6 @@ predicate isDeadCode2QueryMetadata(Query query, string queryId, string ruleId, s ruleId = "RULE-2-8" and category = "advisory" or - query = - // `Query` instance for the `unusedObjectDefinitionInMacro` query - DeadCode2Package::unusedObjectDefinitionInMacroQuery() and - queryId = - // `@id` for the `unusedObjectDefinitionInMacro` query - "c/misra/unused-object-definition-in-macro" and - ruleId = "RULE-2-8" and - category = "advisory" - or query = // `Query` instance for the `unusedObjectDefinitionStrict` query DeadCode2Package::unusedObjectDefinitionStrictQuery() and @@ -36,15 +25,6 @@ predicate isDeadCode2QueryMetadata(Query query, string queryId, string ruleId, s "c/misra/unused-object-definition-strict" and ruleId = "RULE-2-8" and category = "advisory" - or - query = - // `Query` instance for the `unusedObjectDefinitionInMacroStrict` query - DeadCode2Package::unusedObjectDefinitionInMacroStrictQuery() and - queryId = - // `@id` for the `unusedObjectDefinitionInMacroStrict` query - "c/misra/unused-object-definition-in-macro-strict" and - ruleId = "RULE-2-8" and - category = "advisory" } module DeadCode2Package { @@ -55,24 +35,10 @@ module DeadCode2Package { TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionQuery())) } - Query unusedObjectDefinitionInMacroQuery() { - //autogenerate `Query` type - result = - // `Query` type for `unusedObjectDefinitionInMacro` query - TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionInMacroQuery())) - } - Query unusedObjectDefinitionStrictQuery() { //autogenerate `Query` type result = // `Query` type for `unusedObjectDefinitionStrict` query TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionStrictQuery())) } - - Query unusedObjectDefinitionInMacroStrictQuery() { - //autogenerate `Query` type - result = - // `Query` type for `unusedObjectDefinitionInMacroStrict` query - TQueryC(TDeadCode2PackageQuery(TUnusedObjectDefinitionInMacroStrictQuery())) - } } diff --git a/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected index eb55b83924..d9a7fe6a07 100644 --- a/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected +++ b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.expected @@ -1,3 +1,4 @@ +| deduplicatemacroresults.cpp:4:8:4:9 | definition of g1 | Findme var 'g1'. | deduplicatemacroresults.cpp:4:8:4:9 | deduplicatemacroresults.cpp:4:8:4:9 | (ignored) | | deduplicatemacroresults.cpp:10:1:10:34 | SOMETIMES_HAS_RESULTS1(type,name) | Invocation of macro $@ has findme var 'g3'. | deduplicatemacroresults.cpp:6:1:6:52 | deduplicatemacroresults.cpp:6:1:6:52 | SOMETIMES_HAS_RESULTS1 | | deduplicatemacroresults.cpp:13:1:13:34 | SOMETIMES_HAS_RESULTS2(type,name) | Invocation of macro $@ has findme var 'g5'. | deduplicatemacroresults.cpp:7:1:7:53 | deduplicatemacroresults.cpp:7:1:7:53 | SOMETIMES_HAS_RESULTS2 | | deduplicatemacroresults.cpp:15:1:15:50 | #define ALWAYS_HAS_SAME_RESULT() extern findme g6; | Macro ALWAYS_HAS_SAME_RESULT always has findme var named g6 | deduplicatemacroresults.cpp:15:1:15:50 | deduplicatemacroresults.cpp:15:1:15:50 | (ignored) | diff --git a/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql index cd999d72c9..9cae8d4ae8 100644 --- a/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql +++ b/cpp/common/test/library/codingstandards/cpp/alertreporting/DeduplicateMacroResults.ql @@ -22,11 +22,16 @@ module FindMeReportConfig implements MacroReportConfigSig { string getMessageResultInIsolatedExpansion(FindMe f) { result = "Invocation of macro $@ has findme var '" + f.getName() + "'." } + + string getMessageNotInMacro(FindMe f) { + result = "Findme var '" + f.getName() + "'." + + } } import DeduplicateMacroResults import DeduplicateMacroResults::Report -from ReportResultInMacro report +from ReportResult report select report.getPrimaryElement(), report.getMessage(), report.getOptionalPlaceholderLocation(), report.getOptionalPlaceholderMessage() diff --git a/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp b/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp index 3c5d8bca5b..d9b3659bf6 100644 --- a/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp +++ b/cpp/common/test/library/codingstandards/cpp/alertreporting/deduplicatemacroresults.cpp @@ -1,7 +1,7 @@ typedef struct { } findme; -findme g1; // ignore -- not in a macro +findme g1; // baseline report, not in a macro #define SOMETIMES_HAS_RESULTS1(type, name) type name // ignore #define SOMETIMES_HAS_RESULTS2(type, name) type name; // ignore diff --git a/docs/development_handbook.md b/docs/development_handbook.md index 97c615ba2e..83670dbbc8 100644 --- a/docs/development_handbook.md +++ b/docs/development_handbook.md @@ -42,6 +42,7 @@ | 0.32.0 | 2024-05-01 | Luke Cartey | Refer to the user manual for the list of supported standards. | | 0.33.0 | 2024-07-30 | Kristen Newbury | Remove out dated references to codeql modules directory usage. | | 0.34.0 | 2024-08-22 | Kristen Newbury | Remove out dated references to git submodules usage. | +| 0.35.0 | 2025-01-15 | Mike Fairhurst | Add guidance for the addition of 'strict' queries. | ## Scope of work diff --git a/docs/user_manual.md b/docs/user_manual.md index 5e808a1401..ae02555091 100644 --- a/docs/user_manual.md +++ b/docs/user_manual.md @@ -29,7 +29,8 @@ | 0.21.0 | 2024-05-01 | Luke Cartey | Add MISRA C++ 2023 as under development, and clarify MISRA C 2012 coverage. | | 0.22.0 | 2024-10-02 | Luke Cartey | Add MISRA C 2023 as under development, and clarify MISRA C 2012 coverage. | | 0.23.0 | 2024-10-21 | Luke Cartey | Add assembly as a hazard. | -| 0.24.0 | 2024-10-22 | Luke Cartey | Add CodeQL packs as a usable output, update release artifacts list. | +| 0.24.0 | 2024-10-22 | Luke Cartey | Add CodeQL packs as a usable output, update release artifacts list. | +| 0.25.0 | 2025-01-15 | Mike Fairhurst | Add guidance for the usage of 'strict' queries. | ## Release information diff --git a/rule_packages/c/DeadCode2.json b/rule_packages/c/DeadCode2.json index b897f595e6..8b373c31b6 100644 --- a/rule_packages/c/DeadCode2.json +++ b/rule_packages/c/DeadCode2.json @@ -18,19 +18,6 @@ "external/misra/c/2012/amendment4" ] }, - { - "description": "Macros should not have unused object definitions.", - "kind": "problem", - "name": "Project macros should not include unused object definitions", - "precision": "very-high", - "severity": "recommendation", - "short_name": "UnusedObjectDefinitionInMacro", - "tags": [ - "maintainability", - "performance", - "external/misra/c/2012/amendment4" - ] - }, { "description": "A strict query which reports all unused object definitions with '__attribute__((unused))'.", "kind": "problem", @@ -44,20 +31,6 @@ "external/misra/c/2012/amendment4", "external/misra/c/strict" ] - }, - { - "description": "A strict query which reports all unused object definitions in macros with '__attribute__((unused))'.", - "kind": "problem", - "name": "Project macros should not include '__attribute__((unused))' object definitions", - "precision": "very-high", - "severity": "recommendation", - "short_name": "UnusedObjectDefinitionInMacroStrict", - "tags": [ - "maintainability", - "performance", - "external/misra/c/2012/amendment4", - "external/misra/c/strict" - ] } ], "title": "A project should not contain unused object definitions", From 333cc77a08606fc3ecf089791555647b295aef98 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Wed, 15 Jan 2025 20:04:29 -0800 Subject: [PATCH 7/8] Code format --- .../cpp/alertreporting/DeduplicateMacroResults.qll | 10 ++++------ .../src/codingstandards/cpp/deadcode/UnusedObjects.qll | 4 +++- .../cpp/alertreporting/DeduplicateMacroResults.ql | 5 +---- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll index 6462ac855a..c56c40b730 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll @@ -110,7 +110,7 @@ signature module MacroReportConfigSig { * string getMessageResultInIsolatedExpansion(InvalidFoo foo) { * result = "Invocation of macro $@ has invalid foo '" + foo.getName() + "'." * } - * + * * string getMessageNotInMacro(ResultElement element) { * result = "Invalid foo '" + element.getName() + "'." * } @@ -286,9 +286,7 @@ module DeduplicateMacroResults< TReportMacroResultWithVariedName(PrimaryMacroDifferentResultElementInAllInvocations def) or TReportIsolatedMacroResult(IsolatedMacroExpansionWithResultElement def) or TReportNotInMacro(ResultElement def) { - not exists (ResultMacroExpansion macroExpansion | - macroExpansion.getResultElement() = def - ) + not exists(ResultMacroExpansion macroExpansion | macroExpansion.getResultElement() = def) } /** @@ -368,8 +366,8 @@ module DeduplicateMacroResults< ) or ( - this = TReportMacroResultWithSameName(_) - or this = TReportNotInMacro(_) + this = TReportMacroResultWithSameName(_) or + this = TReportNotInMacro(_) ) and result = "(ignored)" or diff --git a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll index fc262b5d93..60e732873a 100644 --- a/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll +++ b/cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll @@ -48,7 +48,9 @@ module ReportDeadObjectConfig implements MacroReportConfigSig { result = "Invocation of macro $@ has findme var '" + f.getName() + "'." } - string getMessageNotInMacro(FindMe f) { - result = "Findme var '" + f.getName() + "'." - - } + string getMessageNotInMacro(FindMe f) { result = "Findme var '" + f.getName() + "'." } } import DeduplicateMacroResults From a8784e1c26bc68622cb16389f9a0e1562db436e5 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Sun, 19 Jan 2025 17:26:38 -0800 Subject: [PATCH 8/8] Simplify DeduplicateMacroResults::Report::ReportResult::toString() for performance reasons --- .../alertreporting/DeduplicateMacroResults.qll | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll index c56c40b730..b3c3d44ff4 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/DeduplicateMacroResults.qll @@ -302,7 +302,21 @@ module DeduplicateMacroResults< * signature parameter. */ class ReportResult extends TReportResult { - string toString() { result = getMessage() } + string toString() { + this = TReportMacroResultWithVariedName(_) and + result = + "Macro that always expands to a result element with invocation-dependent description" + or + this = TReportMacroResultWithSameName(_) and + result = "Macro that always expands to a result element with a constant description" + or + this = TReportIsolatedMacroResult(_) and + result = + "Specific macro expansion which produces a result element, but not all expansions do" + or + this = TReportNotInMacro(_) and + result = "Result element that is not in a macro" + } string getMessage() { exists(PrimaryMacroDifferentResultElementInAllInvocations def |