From 917c62e1b70d9f4de6a32bb3c9fcdc134cf2bdcd Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 8 Mar 2023 11:24:25 +0000 Subject: [PATCH 1/3] Rule 5.8: Improve performance This rule was highlighted as performing poorly as it created an effective cross product on declarations with the same name, which is expensive on some databases. The performance fix is to: (a) Create a predicate which represents all the conflicting identifiers by simply counting the number of declarations and confirming at least on such declaration is external. (b) Use this to create a result table of Declarations that conflict with those identifiers. (c) Implement a "non unique" external identifier class that provides a member predicate to get all the conflicting declarations. The key point here is to prevent the optimiser from doing a join between Declaration.getName() and Declaration.getName(). Instead, the join is between the names of the non-unique external identifiers and the much smaller table of declarations that conflict with at least one such entry. --- ...IdentifiersWithExternalLinkageNotUnique.ql | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/c/misra/src/rules/RULE-5-8/IdentifiersWithExternalLinkageNotUnique.ql b/c/misra/src/rules/RULE-5-8/IdentifiersWithExternalLinkageNotUnique.ql index ff20ceed18..7406f05f14 100644 --- a/c/misra/src/rules/RULE-5-8/IdentifiersWithExternalLinkageNotUnique.ql +++ b/c/misra/src/rules/RULE-5-8/IdentifiersWithExternalLinkageNotUnique.ql @@ -15,10 +15,39 @@ import cpp import codingstandards.c.misra import codingstandards.cpp.Identifiers -from Declaration de, ExternalIdentifiers e +/** + * Holds if the `identifierName` has conflicting declarations. + */ +predicate isExternalIdentifierNotUnique(string identifierName) { + // More than one declaration with this name + count(Declaration d | d.getName() = identifierName) > 1 and + // At least one declaration is an external identifier + exists(ExternalIdentifiers e | e.getName() = identifierName) +} + +/** + * Holds if the `Declaration` `d` is conflicting with an external identifier. + */ +predicate isConflictingDeclaration(Declaration d, string name) { + isExternalIdentifierNotUnique(name) and + d.getName() = name +} + +/** + * An external identifier which is not uniquely defined in the source code. + */ +class NotUniqueExternalIdentifier extends ExternalIdentifiers { + NotUniqueExternalIdentifier() { isExternalIdentifierNotUnique(getName()) } + + Declaration getAConflictingDeclaration() { + not result = this and + isConflictingDeclaration(result, getName()) + } +} + +from NotUniqueExternalIdentifier e, Declaration de where not isExcluded(de, Declarations6Package::identifiersWithExternalLinkageNotUniqueQuery()) and not isExcluded(e, Declarations6Package::identifiersWithExternalLinkageNotUniqueQuery()) and - not de = e and - de.getName() = e.getName() + de = e.getAConflictingDeclaration() select de, "Identifier conflicts with external identifier $@", e, e.getName() From 6dc320552cc24cbf6f98e1519addb222e9d898ff Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 8 Mar 2023 11:43:30 +0000 Subject: [PATCH 2/3] Rule 8.7: Improve performance This rule was identified as containing one of the slowest predicates in our C Coding Standards query suites, and this commit improves the performance. Performance is improved with the following changes: (a) Factoring out a predicate for the repeated calls to getTarget(), to avoid duplication and to create a semantically meaningful predicate which gets the target for a reference to an external identifier. (b) Use the factored out predicate to refine the reference class to only be references to external identifiers. This reduces the size of the class. (c) Create a predicate for computing a table of external identifiers, references to those identifiers and the translation units those exist in. We can then compute this table once, and use it in both the "find me a reference to this external identifier" case and in the "where the external identifier is not referenced in any other translation unit" case. Part (c) is the critical change. Without that, the optimizer was creating an expensive join order in the negation case, where it was effectively creating a cross product of all references to each external identifier, before later excluding on the negation. The use of our predicate in the negation case means we can first create a table of external identifiers and translation units, then apply that in the negation case without cross producting the references. --- .../ShouldNotBeDefinedWithExternalLinkage.ql | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/c/misra/src/rules/RULE-8-7/ShouldNotBeDefinedWithExternalLinkage.ql b/c/misra/src/rules/RULE-8-7/ShouldNotBeDefinedWithExternalLinkage.ql index e5649400c8..824a4cf1cf 100644 --- a/c/misra/src/rules/RULE-8-7/ShouldNotBeDefinedWithExternalLinkage.ql +++ b/c/misra/src/rules/RULE-8-7/ShouldNotBeDefinedWithExternalLinkage.ql @@ -20,27 +20,36 @@ import codingstandards.c.misra import codingstandards.cpp.Identifiers import codingstandards.cpp.Scope +ExternalIdentifiers getExternalIdentifierTarget(NameQualifiableElement nqe) { + result = nqe.(Access).getTarget() + or + result = nqe.(FunctionCall).getTarget() +} + /** - * Re-introduce function calls into access description as - * "any reference" + * A reference to an external identifier, either as an `Access` or a `FunctionCall`. */ -class Reference extends NameQualifiableElement { - Reference() { - this instanceof Access or - this instanceof FunctionCall - } +class ExternalIdentifierReference extends NameQualifiableElement { + ExternalIdentifierReference() { exists(getExternalIdentifierTarget(this)) } + + ExternalIdentifiers getExternalIdentifierTarget() { result = getExternalIdentifierTarget(this) } +} + +predicate isReferencedInTranslationUnit( + ExternalIdentifiers e, ExternalIdentifierReference r, TranslationUnit t +) { + r.getExternalIdentifierTarget() = e and + r.getFile() = t } -from ExternalIdentifiers e, Reference a1, TranslationUnit t1 +from ExternalIdentifiers e, ExternalIdentifierReference a1, TranslationUnit t1 where not isExcluded(e, Declarations6Package::shouldNotBeDefinedWithExternalLinkageQuery()) and - (a1.(Access).getTarget() = e or a1.(FunctionCall).getTarget() = e) and - a1.getFile() = t1 and - //not accessed in any other translation unit - not exists(TranslationUnit t2, Reference a2 | - not t1 = t2 and - (a2.(Access).getTarget() = e or a2.(FunctionCall).getTarget() = e) and - a2.getFile() = t2 + isReferencedInTranslationUnit(e, a1, t1) and + // Not referenced in any other translation unit + not exists(TranslationUnit t2 | + isReferencedInTranslationUnit(e, _, t2) and + not t1 = t2 ) select e, "Declaration with external linkage is accessed in only one translation unit $@.", a1, a1.toString() From 51aa4d03d7c13e0d1e88496b4bf88040fb5cdfff Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 8 Mar 2023 12:00:51 +0000 Subject: [PATCH 3/3] Add change note. --- change_notes/2023-03-08-identifier-performance.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 change_notes/2023-03-08-identifier-performance.md diff --git a/change_notes/2023-03-08-identifier-performance.md b/change_notes/2023-03-08-identifier-performance.md new file mode 100644 index 0000000000..39c2d26bbf --- /dev/null +++ b/change_notes/2023-03-08-identifier-performance.md @@ -0,0 +1,3 @@ + * The performance of the following identifier related rules has been improved: + * MISRA C 2012 `Rule 5.8` + * MISRA C 2012 `Rule 8.7` \ No newline at end of file