-
Notifications
You must be signed in to change notification settings - Fork 72
First implementation of Rule-0-0-2, invariant conditions. #1003
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
Open
MichaelRFairhurst
wants to merge
6
commits into
main
Choose a base branch
from
michaelrfairhurst/deadcode-4-rule-0-0-2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+423
−141
Open
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5a5da3c
First implementation of Rule-0-0-2, invariant conditions.
MichaelRFairhurst 3c8c1c5
s/maintenance/maintainability in metadata tags
MichaelRFairhurst 7c3cbda
Add rules.csv change
MichaelRFairhurst f0c5cd9
Address copilot feedback
MichaelRFairhurst e4a1a69
Fix InvariantCondition.qll comment, test expectations
MichaelRFairhurst 84cad63
Merge branch 'main' into michaelrfairhurst/deadcode-4-rule-0-0-2
MichaelRFairhurst File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| - `M0-1-2` - `InfeasiblePath.ql`: | ||
| - Refactored to share logic with `RULE-0-0-2` while allowing for different exceptional cases. No change in behavior expected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
cpp/common/src/codingstandards/cpp/exclusions/cpp/DeadCode4.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| //** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ | ||
| import cpp | ||
| import RuleMetadata | ||
| import codingstandards.cpp.exclusions.RuleMetadata | ||
|
|
||
| newtype DeadCode4Query = TInvariantConditionQuery() | ||
|
|
||
| predicate isDeadCode4QueryMetadata(Query query, string queryId, string ruleId, string category) { | ||
| query = | ||
| // `Query` instance for the `invariantCondition` query | ||
| DeadCode4Package::invariantConditionQuery() and | ||
| queryId = | ||
| // `@id` for the `invariantCondition` query | ||
| "cpp/misra/invariant-condition" and | ||
| ruleId = "RULE-0-0-2" and | ||
| category = "advisory" | ||
| } | ||
|
|
||
| module DeadCode4Package { | ||
| Query invariantConditionQuery() { | ||
| //autogenerate `Query` type | ||
| result = | ||
| // `Query` type for `invariantCondition` query | ||
| TQueryCPP(TDeadCode4PackageQuery(TInvariantConditionQuery())) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
159 changes: 159 additions & 0 deletions
159
cpp/common/src/codingstandards/cpp/rules/invariantcondition/InvariantCondition.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| /** | ||
| * Provides a library which includes a `problems` predicate for reporting | ||
| * invariant/infeasible code paths. | ||
| */ | ||
|
|
||
| import cpp | ||
| import codingstandards.cpp.Customizations | ||
| import codingstandards.cpp.Exclusions | ||
| import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis | ||
| import codingstandards.cpp.deadcode.UnreachableCode | ||
| import semmle.code.cpp.controlflow.Guards | ||
|
|
||
| signature module InvariantConditionConfigSig { | ||
| Query getQuery(); | ||
|
|
||
| predicate isException(ControlFlowNode element); | ||
| } | ||
|
|
||
| private module Impl { | ||
| /** | ||
| * A "conditional" node in the control flow graph, i.e. one that can potentially have a true and false path. | ||
| */ | ||
| class ConditionalControlFlowNode extends ControlFlowNode { | ||
| ConditionalControlFlowNode() { | ||
| // A conditional node is one with at least one of a true or false successor | ||
| (exists(getATrueSuccessor()) or exists(getAFalseSuccessor())) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Holds if the `ConditionalNode` has an infeasible `path` according to the control flow graph library. | ||
| */ | ||
| predicate hasCFGDeducedInfeasiblePath( | ||
| ConditionalControlFlowNode cond, boolean infeasiblePath, string explanation | ||
| ) { | ||
| not cond.isFromTemplateInstantiation(_) and | ||
| // No true successor, so the true path has already been deduced as infeasible | ||
| not exists(cond.getATrueSuccessor()) and | ||
| infeasiblePath = true and | ||
| explanation = "this expression consists of constants which evaluate to false" | ||
| or | ||
| // No false successor, so false path has already been deduced as infeasible | ||
| not exists(cond.getAFalseSuccessor()) and | ||
| infeasiblePath = false and | ||
| explanation = "this expression consists of constants which evaluate to true" | ||
| } | ||
|
|
||
| predicate isConstantRelationalOperation( | ||
| RelationalOperation rel, boolean infeasiblePath, string explanation | ||
| ) { | ||
| /* | ||
| * This predicate identifies a number of a cases where we can conclusive determine that a relational | ||
| * operation will always return true or false, based on the ranges for each operand as determined | ||
| * by the SimpleRangeAnalysis library (and any extensions provide in the Coding Standards library). | ||
MichaelRFairhurst marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * | ||
| * Important note: in order to deduce that an relational operation _always_ returns true or false, | ||
MichaelRFairhurst marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * we must ensure that it returns true or false for _all_ possible values of the operands. For | ||
| * example, it may be tempting to look at this relational operation on these ranges: | ||
| * ``` | ||
| * [0..5] < [0..10] | ||
| * ``` | ||
| * And say that ub(lesser) < ub(greater) and therefore it is `true`, however this is not the case | ||
| * for all permutations (e.g. 5 < 0). | ||
| * | ||
| * Instead, we look at all four permutations of these two dimensions: | ||
| * - Equal-to or not equal-to | ||
| * - Always true or always false | ||
| */ | ||
|
|
||
| // This restricts the comparison to occur directly within the conditional node | ||
| // In theory we could also extend this to identify comparisons where the result is stored, then | ||
| // later read in a conditional control flow node within the same function (using SSA) | ||
| // Doing so would benefit from path explanations, but would require a more complex analysis | ||
| rel instanceof ConditionalControlFlowNode and | ||
| // If at least one operand includes an access of a volatile variable, the range analysis library may | ||
| // provide inaccurate results, so we ignore this case | ||
| not rel.getAnOperand().getAChild*().(VariableAccess).getTarget().isVolatile() and | ||
| exists(boolean isEqual | | ||
| if | ||
| rel instanceof GEExpr | ||
| or | ||
| rel instanceof LEExpr | ||
| then isEqual = true | ||
| else isEqual = false | ||
| | | ||
| // Not equal-to/always true | ||
| // If the largest value of the lesser operand is less than the smallest value of the greater | ||
| // operand, then the LT/GT comparison is always true | ||
| // Example: [0..5] < [6..10] | ||
| upperBound(rel.getLesserOperand()) < lowerBound(rel.getGreaterOperand()) and | ||
| explanation = | ||
| rel.getLesserOperand() + " (max value: " + upperBound(rel.getLesserOperand()) + | ||
| ") is always less than " + rel.getGreaterOperand() + " (minimum value: " + | ||
| lowerBound(rel.getGreaterOperand()) + ")" and | ||
| isEqual = false and | ||
| infeasiblePath = false | ||
| or | ||
| // Equal-to/always true | ||
| // If the largest value of the lesser operand is less than or equal to the smallest value of the | ||
| // greater operand, then the LTE/GTE comparison is always true | ||
| // Example: [0..6] <= [6..10] | ||
| upperBound(rel.getLesserOperand()) <= lowerBound(rel.getGreaterOperand()) and | ||
| explanation = | ||
| rel.getLesserOperand() + " (max value: " + upperBound(rel.getLesserOperand()) + | ||
| ") is always less than or equal to " + rel.getGreaterOperand() + " (minimum value: " + | ||
| lowerBound(rel.getGreaterOperand()) + ")" and | ||
| isEqual = true and | ||
| infeasiblePath = false | ||
| or | ||
| // Equal to/always false | ||
| // If the largest value of the greater operand is less than the smallest value of the lesser | ||
| // operand, then the LTE/GTE comparison is always false | ||
| // Example: [6..10] <= [0..5] | ||
| upperBound(rel.getGreaterOperand()) < lowerBound(rel.getLesserOperand()) and | ||
| explanation = | ||
| rel.getGreaterOperand() + " (max value: " + upperBound(rel.getGreaterOperand()) + | ||
| ") is always less than " + rel.getLesserOperand() + " (minimum value: " + | ||
| lowerBound(rel.getLesserOperand()) + ")" and | ||
| isEqual = true and | ||
| infeasiblePath = true | ||
| or | ||
| // Equal to/always true | ||
MichaelRFairhurst marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // If the largest value of the greater operand is less than or equal to the smallest value of the | ||
| // lesser operand, then the LT/GT comparison is always false | ||
| // Example: [6..10] < [0..6] | ||
| upperBound(rel.getGreaterOperand()) <= lowerBound(rel.getLesserOperand()) and | ||
| explanation = | ||
| rel.getGreaterOperand() + " (max value: " + upperBound(rel.getGreaterOperand()) + | ||
| ") is always less than or equal to " + rel.getLesserOperand() + " (minimum value: " + | ||
| lowerBound(rel.getLesserOperand()) + ")" and | ||
| isEqual = false and | ||
| infeasiblePath = true | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Holds if the `ConditionalNode` has an infeasible `path` for the reason given in `explanation`. | ||
| */ | ||
| predicate hasInfeasiblePath(ConditionalControlFlowNode node, string message) { | ||
| exists(boolean infeasiblePath, string explanation | | ||
| if node.isFromUninstantiatedTemplate(_) | ||
| then message = "The path is unreachable in a template." | ||
| else message = "The " + infeasiblePath + " path is infeasible because " + explanation + "." | ||
| | | ||
| hasCFGDeducedInfeasiblePath(node, infeasiblePath, explanation) and | ||
| not isConstantRelationalOperation(node, infeasiblePath, _) | ||
| or | ||
| isConstantRelationalOperation(node, infeasiblePath, explanation) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| module InvariantCondition<InvariantConditionConfigSig Config> { | ||
| query predicate problems(Impl::ConditionalControlFlowNode cond, string explanation) { | ||
| not isExcluded(cond, Config::getQuery()) and | ||
| Impl::hasInfeasiblePath(cond, explanation) and | ||
| not Config::isException(cond) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.