From f5437d2d8c38e2fd8711c8db912713cdba1d35a8 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 13:51:23 +0100 Subject: [PATCH 1/8] Add an "Includes" library for detecting conditional includes --- .../src/codingstandards/cpp/Includes.qll | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 cpp/common/src/codingstandards/cpp/Includes.qll diff --git a/cpp/common/src/codingstandards/cpp/Includes.qll b/cpp/common/src/codingstandards/cpp/Includes.qll new file mode 100644 index 0000000000..bb72dafeda --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/Includes.qll @@ -0,0 +1,31 @@ +/** A library which supports analysis of includes. */ + +import cpp +import semmle.code.cpp.headers.MultipleInclusion + +/** + * Holds if `include` is included conditionally based on the branch directive `b1`. + */ +predicate conditionallyIncluded(PreprocessorBranchDirective b1, Include include) { + exists(File f, int include1StartLine | + not b1 = any(CorrectIncludeGuard c).getIfndef() and + not b1.getHead().regexpMatch(".*_H(_.*)?") and + include.getLocation().hasLocationInfo(f.getAbsolutePath(), include1StartLine, _, _, _) and + f.getAbsolutePath() = b1.getFile().getAbsolutePath() + | + b1.getLocation().getStartLine() < include1StartLine and + b1.getNext().getLocation().getStartLine() > include1StartLine + ) +} + +/** + * Gets a file which is directly included from `fromFile` unconditionally. + */ +File getAnUnconditionallyIncludedFile(File fromFile) { + // Find an include which isn't conditional + exists(Include i | + i.getFile() = fromFile and + not conditionallyIncluded(_, i) and + result = i.getIncludedFile() + ) +} From 650842bce1e1a1245913fdcd15db95c8248d46ea Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 13:51:58 +0100 Subject: [PATCH 2/8] PreprocessDirectives: Add detection for conditionally defined macros Add helper predicates for identifying macros defined in conditional blocks, and pairs of macros which are mutually exclusive. --- .../cpp/PreprocessorDirective.qll | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll index fe619e5317..5f360e4d31 100644 --- a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll +++ b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll @@ -40,3 +40,70 @@ class PreprocessorIfOrElif extends PreprocessorBranch { this instanceof PreprocessorElif } } + +/** + * Holds if the preprocessor directive `m` is located at `filepath` and `startline`. + */ +private predicate hasPreprocessorLocation(PreprocessorDirective m, string filepath, int startline) { + m.getLocation().hasLocationInfo(filepath, startline, _, _, _) +} + +/** + * Holds if `first` and `second` are a pair of branch directives in the same file, such that they + * share the same root if condition. + */ +pragma[noinline] +private predicate isBranchDirectivePair( + PreprocessorBranchDirective first, PreprocessorBranchDirective second, string filepath, + int b1StartLocation, int b2StartLocation +) { + first.getIf() = second.getIf() and + not first = second and + hasPreprocessorLocation(first, filepath, b1StartLocation) and + hasPreprocessorLocation(second, filepath, b2StartLocation) and + b1StartLocation < b2StartLocation +} + +/** + * Holds if `bd` is a branch directive in the range `filepath`, `startline`, `endline`. + */ +pragma[noinline] +predicate isBranchDirectiveRange( + PreprocessorBranchDirective bd, string filepath, int startline, int endline +) { + hasPreprocessorLocation(bd, filepath, startline) and + exists(PreprocessorBranchDirective next | + next = bd.getNext() and + // Avoid referencing filepath here, otherwise the optimiser will try to join + // on it + hasPreprocessorLocation(next, _, endline) + ) +} + +/** + * Holds if the macro `m` is defined within the branch directive `bd`. + */ +pragma[noinline] +predicate isMacroDefinedWithinBranch(PreprocessorBranchDirective bd, Macro m) { + exists(string filepath, int startline, int endline, int macroline | + isBranchDirectiveRange(bd, filepath, startline, endline) and + hasPreprocessorLocation(m, filepath, macroline) and + startline < macroline and + endline > macroline + ) +} + +/** + * Holds if the pair of macros are "conditional" i.e. only one of the macros is followed in any + * particular compilation of the containing file. + */ +predicate mutuallyExclusiveMacros(Macro firstMacro, Macro secondMacro) { + exists( + PreprocessorBranchDirective b1, PreprocessorBranchDirective b2, string filepath, + int b1StartLocation, int b2StartLocation + | + isBranchDirectivePair(b1, b2, filepath, b1StartLocation, b2StartLocation) and + isMacroDefinedWithinBranch(b1, firstMacro) and + isMacroDefinedWithinBranch(b2, secondMacro) + ) +} From 15430197f9c409cc0e4fb2ca4aacfdac6c43b0c8 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 14:14:26 +0100 Subject: [PATCH 3/8] Includes: Improve performance --- .../src/codingstandards/cpp/Includes.qll | 26 ++++++++++++------- .../cpp/PreprocessorDirective.qll | 3 ++- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/Includes.qll b/cpp/common/src/codingstandards/cpp/Includes.qll index bb72dafeda..c0c66ae2f5 100644 --- a/cpp/common/src/codingstandards/cpp/Includes.qll +++ b/cpp/common/src/codingstandards/cpp/Includes.qll @@ -1,20 +1,26 @@ /** A library which supports analysis of includes. */ import cpp +import codingstandards.cpp.PreprocessorDirective import semmle.code.cpp.headers.MultipleInclusion +pragma[noinline] +private predicate hasIncludeLocation(Include include, string filepath, int startline) { + include.getLocation().hasLocationInfo(filepath, startline, _, _, _) +} + /** * Holds if `include` is included conditionally based on the branch directive `b1`. */ -predicate conditionallyIncluded(PreprocessorBranchDirective b1, Include include) { - exists(File f, int include1StartLine | - not b1 = any(CorrectIncludeGuard c).getIfndef() and - not b1.getHead().regexpMatch(".*_H(_.*)?") and - include.getLocation().hasLocationInfo(f.getAbsolutePath(), include1StartLine, _, _, _) and - f.getAbsolutePath() = b1.getFile().getAbsolutePath() - | - b1.getLocation().getStartLine() < include1StartLine and - b1.getNext().getLocation().getStartLine() > include1StartLine +pragma[noinline] +predicate isConditionallyIncluded(PreprocessorBranchDirective bd, Include include) { + not bd = any(CorrectIncludeGuard c).getIfndef() and + not bd.getHead().regexpMatch(".*_H(_.*)?") and + exists(string filepath, int startline, int endline, int includeline | + isBranchDirectiveRange(bd, filepath, startline, endline) and + hasIncludeLocation(include, filepath, includeline) and + startline < includeline and + endline > includeline ) } @@ -25,7 +31,7 @@ File getAnUnconditionallyIncludedFile(File fromFile) { // Find an include which isn't conditional exists(Include i | i.getFile() = fromFile and - not conditionallyIncluded(_, i) and + not isConditionallyIncluded(_, i) and result = i.getIncludedFile() ) } diff --git a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll index 5f360e4d31..1f6cc140d8 100644 --- a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll +++ b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll @@ -44,7 +44,8 @@ class PreprocessorIfOrElif extends PreprocessorBranch { /** * Holds if the preprocessor directive `m` is located at `filepath` and `startline`. */ -private predicate hasPreprocessorLocation(PreprocessorDirective m, string filepath, int startline) { +pragma[noinline] +predicate hasPreprocessorLocation(PreprocessorDirective m, string filepath, int startline) { m.getLocation().hasLocationInfo(filepath, startline, _, _, _) } From 34abb425d726de2f6abd9095108620136642121d Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 15:23:23 +0100 Subject: [PATCH 4/8] Rule 5.4: Exclude cases where the two macros are conditional - Macros in #ifndef MACRO_NAME blocks - Pairs of macros that exists in different #if/#elif/#else cases - Pairs of macros defined in files that are mutually exclusively included. - If the macros are used, they must be both occur in the same link target. --- .../RULE-5-4/MacroIdentifiersNotDistinct.ql | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql index abd22068dd..8399524984 100644 --- a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql +++ b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql @@ -15,6 +15,25 @@ import cpp import codingstandards.c.misra +import codingstandards.cpp.Macro +import codingstandards.cpp.Includes +import codingstandards.cpp.PreprocessorDirective + +/** + * Gets a link target that this macro is expanded in. + */ +LinkTarget getALinkTarget(Macro m) { + exists(Element e | e = m.getAnInvocation().getAnAffectedElement() | + result = e.(Expr).getEnclosingFunction().getALinkTarget() + or + result = e.(Stmt).getEnclosingFunction().getALinkTarget() + or + exists(GlobalOrNamespaceVariable g | + result = g.getALinkTarget() and + g.getInitializer().getExpr().getAChild*() = e + ) + ) +} from Macro m, Macro m2 where @@ -30,7 +49,31 @@ where else m.getName() = m2.getName() ) and //reduce double report since both macros are in alert, arbitrary ordering - m.getLocation().getStartLine() >= m2.getLocation().getStartLine() + m.getLocation().getStartLine() >= m2.getLocation().getStartLine() and + // Not within an #ifndef MACRO_NAME + not exists(PreprocessorIfndef ifBranch | + m.getAGuard() = ifBranch or + m2.getAGuard() = ifBranch + | + ifBranch.getHead() = m.getName() + ) and + // Must be included unconditionally from the same file, otherwise m1 may not be defined + // when m2 is defined + exists(File f | + getAnUnconditionallyIncludedFile*(f) = m.getFile() and + getAnUnconditionallyIncludedFile*(f) = m2.getFile() + ) and + // Macros can't be mutually exclusive + not mutuallyExclusiveMacros(m, m2) and + not mutuallyExclusiveMacros(m2, m) and + // If at least one invocation exists for at least one of the macros, then they must share a link + // target - i.e. must both be expanded in the same context + ( + (exists(m.getAnInvocation()) and exists(m2.getAnInvocation())) + implies + // Must share a link target - e.g. must both be expanded in the same context + getALinkTarget(m) = getALinkTarget(m2) + ) select m, "Macro identifer " + m.getName() + " is nondistinct in first 63 characters, compared to $@.", m2, m2.getName() From 08c0aafd8ba03c62643f046ddcbf3ba41dff8868 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 16:42:30 +0100 Subject: [PATCH 5/8] Rule 5.4: add conditional test cases --- .../MacroIdentifiersNotDistinct.expected | 2 ++ c/misra/test/rules/RULE-5-4/conditional.h | 11 +++++++++++ c/misra/test/rules/RULE-5-4/header1.h | 1 + c/misra/test/rules/RULE-5-4/header2.h | 1 + c/misra/test/rules/RULE-5-4/header3.h | 16 ++++++++++++++++ c/misra/test/rules/RULE-5-4/header4.h | 13 +++++++++++++ c/misra/test/rules/RULE-5-4/root1.c | 6 ++++++ c/misra/test/rules/RULE-5-4/root2.c | 3 +++ 8 files changed, 53 insertions(+) create mode 100644 c/misra/test/rules/RULE-5-4/conditional.h create mode 100644 c/misra/test/rules/RULE-5-4/header1.h create mode 100644 c/misra/test/rules/RULE-5-4/header2.h create mode 100644 c/misra/test/rules/RULE-5-4/header3.h create mode 100644 c/misra/test/rules/RULE-5-4/header4.h create mode 100644 c/misra/test/rules/RULE-5-4/root1.c create mode 100644 c/misra/test/rules/RULE-5-4/root2.c diff --git a/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected b/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected index 12507b2d3f..d31f32acb2 100644 --- a/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected +++ b/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected @@ -1,2 +1,4 @@ +| header3.h:7:1:7:24 | #define MULTIPLE_INCLUDE | Macro identifer MULTIPLE_INCLUDE is nondistinct in first 63 characters, compared to $@. | header4.h:1:1:1:24 | #define MULTIPLE_INCLUDE | MULTIPLE_INCLUDE | +| header3.h:14:1:14:21 | #define NOT_PROTECTED | Macro identifer NOT_PROTECTED is nondistinct in first 63 characters, compared to $@. | header4.h:12:1:12:23 | #define NOT_PROTECTED 1 | NOT_PROTECTED | | test.c:2:1:2:72 | #define iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyB | Macro identifer iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyB is nondistinct in first 63 characters, compared to $@. | test.c:1:1:1:72 | #define iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyA | iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyA | | test.c:8:1:8:31 | #define FUNCTION_MACRO(X) X + 1 | Macro identifer FUNCTION_MACRO is nondistinct in first 63 characters, compared to $@. | test.c:7:1:7:57 | #define FUNCTION_MACRO(FUNCTION_MACRO) FUNCTION_MACRO + 1 | FUNCTION_MACRO | diff --git a/c/misra/test/rules/RULE-5-4/conditional.h b/c/misra/test/rules/RULE-5-4/conditional.h new file mode 100644 index 0000000000..d30701c8e0 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/conditional.h @@ -0,0 +1,11 @@ +#ifdef FOO +#include "header1.h" +#else +#include "header2.h" +#endif + +#ifdef FOO +#define A_MACRO 1 // COMPLIANT +#else +#define A_MACRO 2 // COMPLIANT +#endif \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header1.h b/c/misra/test/rules/RULE-5-4/header1.h new file mode 100644 index 0000000000..526f4fa659 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header1.h @@ -0,0 +1 @@ +#define REPEATED 11 // COMPLIANT \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header2.h b/c/misra/test/rules/RULE-5-4/header2.h new file mode 100644 index 0000000000..bd5dde123d --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header2.h @@ -0,0 +1 @@ +#define REPEATED 1 // COMPLIANT \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header3.h b/c/misra/test/rules/RULE-5-4/header3.h new file mode 100644 index 0000000000..3aa82f5fd7 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header3.h @@ -0,0 +1,16 @@ +#ifndef HEADER3_H +#define HEADER3_H + +// We should ignore the header guards in this file + +// This is defined unconditionally by both header3.h and header4.h +#define MULTIPLE_INCLUDE // NON_COMPLIANT + +// This is redefined in header3.h, but only if it isn't already defined +#define PROTECTED // COMPLIANT + +// This is redefined in header3.h, but is conditional on some other condition, +// so this is redefined +#define NOT_PROTECTED // NON_COMPLIANT + +#endif \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/header4.h b/c/misra/test/rules/RULE-5-4/header4.h new file mode 100644 index 0000000000..8fa6e8b5e8 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/header4.h @@ -0,0 +1,13 @@ +#define MULTIPLE_INCLUDE // NON_COMPLIANT + +// This case is triggered from root2.c +// because PROTECTED isn't defined in +// that case +#ifndef PROTECTED +#define PROTECTED // COMPLIANT - checked by guard +#endif + +// Always enabled, so conflicts in root1.c case +#ifdef MULTIPLE_INCLUDE +#define NOT_PROTECTED 1 // NON_COMPLIANT +#endif diff --git a/c/misra/test/rules/RULE-5-4/root1.c b/c/misra/test/rules/RULE-5-4/root1.c new file mode 100644 index 0000000000..98a94abed5 --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/root1.c @@ -0,0 +1,6 @@ +#define FOO 1 +#include "conditional.h" + +// Both headers define MULTIPLE_INCLUDE +#include "header3.h" +#include "header4.h" \ No newline at end of file diff --git a/c/misra/test/rules/RULE-5-4/root2.c b/c/misra/test/rules/RULE-5-4/root2.c new file mode 100644 index 0000000000..39926b9b1e --- /dev/null +++ b/c/misra/test/rules/RULE-5-4/root2.c @@ -0,0 +1,3 @@ +#include "conditional.h" + +#include "header4.h" \ No newline at end of file From 671620fb96e777fb38c90fb769f6c13190a0929c Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 21:15:02 +0100 Subject: [PATCH 6/8] Rule 5.4: Improve alert message, ensure efficient performance --- .../RULE-5-4/MacroIdentifiersNotDistinct.ql | 59 +++++++++++++++---- .../MacroIdentifiersNotDistinct.expected | 6 +- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql index 8399524984..ed124692e5 100644 --- a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql +++ b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql @@ -19,23 +19,52 @@ import codingstandards.cpp.Macro import codingstandards.cpp.Includes import codingstandards.cpp.PreprocessorDirective +/** + * Gets a top level element that this macro is expanded to, e.g. an element which does not also have + * an enclosing element in the macro. + */ +Element getATopLevelElement(MacroInvocation mi) { + result = mi.getAnExpandedElement() and + not result.getEnclosingElement() = mi.getAnExpandedElement() and + not result instanceof Conversion +} + /** * Gets a link target that this macro is expanded in. */ LinkTarget getALinkTarget(Macro m) { - exists(Element e | e = m.getAnInvocation().getAnAffectedElement() | + exists(MacroInvocation mi, Element e | + mi = m.getAnInvocation() and + e = getATopLevelElement(mi) + | result = e.(Expr).getEnclosingFunction().getALinkTarget() or result = e.(Stmt).getEnclosingFunction().getALinkTarget() or exists(GlobalOrNamespaceVariable g | result = g.getALinkTarget() and - g.getInitializer().getExpr().getAChild*() = e + g = e.(Expr).getEnclosingDeclaration() ) ) } -from Macro m, Macro m2 +/** + * Holds if the m1 and m2 are unconditionally included from a common file. + * + * Extracted out for performance reasons - otherwise the call to determine the file path for the + * message was specializing the calls to `getAnUnconditionallyIncludedFile*(..)` and causing + * slow performance. + */ +bindingset[m1, m2] +pragma[inline_late] +private predicate isIncludedUnconditionallyFromCommonFile(Macro m1, Macro m2) { + exists(File f | + getAnUnconditionallyIncludedFile*(f) = m1.getFile() and + getAnUnconditionallyIncludedFile*(f) = m2.getFile() + ) +} + +from Macro m, Macro m2, string message where not isExcluded(m, Declarations1Package::macroIdentifiersNotDistinctQuery()) and not m = m2 and @@ -44,9 +73,18 @@ where //C90 states the first 31 characters of macro identifiers are significant and is not currently considered by this rule //ie an identifier differing on the 32nd character would be indistinct for C90 but distinct for C99 //and is currently not reported by this rule - if m.getName().length() >= 64 - then m.getName().prefix(63) = m2.getName().prefix(63) - else m.getName() = m2.getName() + if m.getName().length() >= 64 and not m.getName() = m2.getName() + then ( + m.getName().prefix(63) = m2.getName().prefix(63) and + message = + "Macro identifer " + m.getName() + " is nondistinct in first 63 characters, compared to $@." + ) else ( + m.getName() = m2.getName() and + message = + "Definition of macro " + m.getName() + + " is not distinct from alternative definition of $@ in " + + m2.getLocation().getFile().getRelativePath() + "." + ) ) and //reduce double report since both macros are in alert, arbitrary ordering m.getLocation().getStartLine() >= m2.getLocation().getStartLine() and @@ -59,10 +97,7 @@ where ) and // Must be included unconditionally from the same file, otherwise m1 may not be defined // when m2 is defined - exists(File f | - getAnUnconditionallyIncludedFile*(f) = m.getFile() and - getAnUnconditionallyIncludedFile*(f) = m2.getFile() - ) and + isIncludedUnconditionallyFromCommonFile(m, m2) and // Macros can't be mutually exclusive not mutuallyExclusiveMacros(m, m2) and not mutuallyExclusiveMacros(m2, m) and @@ -74,6 +109,4 @@ where // Must share a link target - e.g. must both be expanded in the same context getALinkTarget(m) = getALinkTarget(m2) ) -select m, - "Macro identifer " + m.getName() + " is nondistinct in first 63 characters, compared to $@.", m2, - m2.getName() +select m, message, m2, m2.getName() diff --git a/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected b/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected index d31f32acb2..d44164d116 100644 --- a/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected +++ b/c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected @@ -1,4 +1,4 @@ -| header3.h:7:1:7:24 | #define MULTIPLE_INCLUDE | Macro identifer MULTIPLE_INCLUDE is nondistinct in first 63 characters, compared to $@. | header4.h:1:1:1:24 | #define MULTIPLE_INCLUDE | MULTIPLE_INCLUDE | -| header3.h:14:1:14:21 | #define NOT_PROTECTED | Macro identifer NOT_PROTECTED is nondistinct in first 63 characters, compared to $@. | header4.h:12:1:12:23 | #define NOT_PROTECTED 1 | NOT_PROTECTED | +| header3.h:7:1:7:24 | #define MULTIPLE_INCLUDE | Definition of macro MULTIPLE_INCLUDE is not distinct from alternative definition of $@ in rules/RULE-5-4/header4.h. | header4.h:1:1:1:24 | #define MULTIPLE_INCLUDE | MULTIPLE_INCLUDE | +| header3.h:14:1:14:21 | #define NOT_PROTECTED | Definition of macro NOT_PROTECTED is not distinct from alternative definition of $@ in rules/RULE-5-4/header4.h. | header4.h:12:1:12:23 | #define NOT_PROTECTED 1 | NOT_PROTECTED | | test.c:2:1:2:72 | #define iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyB | Macro identifer iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyB is nondistinct in first 63 characters, compared to $@. | test.c:1:1:1:72 | #define iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyA | iltiqzxgfqsgigwfuyntzghvzltueatcxqnqofnnvjyszmcsylyohvqaosjbqyyA | -| test.c:8:1:8:31 | #define FUNCTION_MACRO(X) X + 1 | Macro identifer FUNCTION_MACRO is nondistinct in first 63 characters, compared to $@. | test.c:7:1:7:57 | #define FUNCTION_MACRO(FUNCTION_MACRO) FUNCTION_MACRO + 1 | FUNCTION_MACRO | +| test.c:8:1:8:31 | #define FUNCTION_MACRO(X) X + 1 | Definition of macro FUNCTION_MACRO is not distinct from alternative definition of $@ in rules/RULE-5-4/test.c. | test.c:7:1:7:57 | #define FUNCTION_MACRO(FUNCTION_MACRO) FUNCTION_MACRO + 1 | FUNCTION_MACRO | From 892cb965e612dbf8c602fadfdea59499e5cf8bf3 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 21:15:30 +0100 Subject: [PATCH 7/8] Add change note --- change_notes/2024-10-21-rule-5-4-conditional.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 change_notes/2024-10-21-rule-5-4-conditional.md diff --git a/change_notes/2024-10-21-rule-5-4-conditional.md b/change_notes/2024-10-21-rule-5-4-conditional.md new file mode 100644 index 0000000000..cfc22f3642 --- /dev/null +++ b/change_notes/2024-10-21-rule-5-4-conditional.md @@ -0,0 +1,3 @@ + - `RULE-5-4` - `MacroIdentifiersNotDistinct.ql`: + - Exclude false positives related to conditional compilation, where a macro may be defined twice, but not within the same compilation. + - Improve alert message in the case the 63 char limit is not relevant by using the form "Definition of macro `` is not distinct from alternative definition of `` in ``. \ No newline at end of file From 57dd7485f67c15a414d079a8b5a15902cf3c5b7b Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 22 Oct 2024 21:18:01 +0100 Subject: [PATCH 8/8] Rule 5.4: Address review comments --- .../src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql | 4 ++-- .../src/codingstandards/cpp/PreprocessorDirective.qll | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql index ed124692e5..36b946491b 100644 --- a/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql +++ b/c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql @@ -99,8 +99,8 @@ where // when m2 is defined isIncludedUnconditionallyFromCommonFile(m, m2) and // Macros can't be mutually exclusive - not mutuallyExclusiveMacros(m, m2) and - not mutuallyExclusiveMacros(m2, m) and + not mutuallyExclusiveBranchDirectiveMacros(m, m2) and + not mutuallyExclusiveBranchDirectiveMacros(m2, m) and // If at least one invocation exists for at least one of the macros, then they must share a link // target - i.e. must both be expanded in the same context ( diff --git a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll index 1f6cc140d8..ca943742d9 100644 --- a/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll +++ b/cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll @@ -98,12 +98,9 @@ predicate isMacroDefinedWithinBranch(PreprocessorBranchDirective bd, Macro m) { * Holds if the pair of macros are "conditional" i.e. only one of the macros is followed in any * particular compilation of the containing file. */ -predicate mutuallyExclusiveMacros(Macro firstMacro, Macro secondMacro) { - exists( - PreprocessorBranchDirective b1, PreprocessorBranchDirective b2, string filepath, - int b1StartLocation, int b2StartLocation - | - isBranchDirectivePair(b1, b2, filepath, b1StartLocation, b2StartLocation) and +predicate mutuallyExclusiveBranchDirectiveMacros(Macro firstMacro, Macro secondMacro) { + exists(PreprocessorBranchDirective b1, PreprocessorBranchDirective b2 | + isBranchDirectivePair(b1, b2, _, _, _) and isMacroDefinedWithinBranch(b1, firstMacro) and isMacroDefinedWithinBranch(b2, secondMacro) )