Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RULE-2-8, project should not contain unused object definitions #784

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions c/misra/src/rules/RULE-2-8/UnusedObjectDefinition.ql
Original file line number Diff line number Diff line change
@@ -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()
24 changes: 24 additions & 0 deletions c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql
Original file line number Diff line number Diff line change
@@ -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()
27 changes: 27 additions & 0 deletions c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql
Original file line number Diff line number Diff line change
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to use the tag "strict" here to cover these cases. Although we've added strict tags before (to AUTOSAR queries), we've never integrated them into our query suites, or provided user facing recommendations around them.

There are a few ways we could consider doing so, but I think my preference would be to:

  1. Exclude strict queries from the default query suites (for only MISRA C to start with - the AUTOSAR strict queries would need to be re-reviewed)
  2. Provide a strict query suite separately.
  3. Update the user_manual.md to discuss the option of "strict" mode.
  4. Update the development_handbook.md to discuss the criteria for a query to be considered for "strict" mode.

We should also, in this case, put into the "implementation_scope" why we consider this to be a "strict" query - i.e. why it is safe to default disable it.

The alternative to a "strict mode tag", that I think is worth mentioning here, is to provide a default "deviations" configuration that disables such queries that we would consider to be "strict".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offered a strict definition of what should be considered a strict query, hopefully it LGTY

* 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()
26 changes: 26 additions & 0 deletions c/misra/src/rules/RULE-2-8/UnusedObjectDefinitionStrict.ql
Original file line number Diff line number Diff line change
@@ -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()
8 changes: 8 additions & 0 deletions c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected
Original file line number Diff line number Diff line change
@@ -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 |
1 change: 1 addition & 0 deletions c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.qlref
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rules/RULE-2-8/UnusedObjectDefinition.ql
Original file line number Diff line number Diff line change
@@ -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) |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rules/RULE-2-8/UnusedObjectDefinitionInMacro.ql
Original file line number Diff line number Diff line change
@@ -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) |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rules/RULE-2-8/UnusedObjectDefinitionInMacroStrict.ql
Original file line number Diff line number Diff line change
@@ -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 |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rules/RULE-2-8/UnusedObjectDefinitionStrict.ql
113 changes: 113 additions & 0 deletions c/misra/test/rules/RULE-2-8/test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// Not a definition, only a declaration:
MichaelRFairhurst marked this conversation as resolved.
Show resolved Hide resolved
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
}
28 changes: 25 additions & 3 deletions cpp/common/src/codingstandards/cpp/AlertReporting.qll
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,46 @@ module MacroUnwrapper<ResultType ResultElement> {
}

/**
* 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.
*/
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) }
}
}
Loading
Loading