Merge pull request #769 from github/lcartey/rule-5-4-conditional-inclusion

`RULE-5-4`: Exclude results which do not occur in the same compilation, improve alert message

Author: nicolaswill

c/misra/src/rules/RULE-5-4/MacroIdentifiersNotDistinct.ql

@@ -15,8 +15,56 @@
 
 import cpp
 import codingstandards.c.misra
+import codingstandards.cpp.Macro
+import codingstandards.cpp.Includes
+import codingstandards.cpp.PreprocessorDirective
 
-from Macro m, Macro m2
+/**
+ * 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(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 = e.(Expr).getEnclosingDeclaration()
+    )
+  )
+}
+
+/**
+ * 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
@@ -25,12 +73,40 @@ 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()
-select m,
-  "Macro identifer " + m.getName() + " is nondistinct in first 63 characters, compared to $@.", m2,
-  m2.getName()
+  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
+  isIncludedUnconditionallyFromCommonFile(m, m2) and
+  // Macros can't be mutually exclusive
+  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
+  (
+    (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, message, m2, m2.getName()

c/misra/test/rules/RULE-5-4/MacroIdentifiersNotDistinct.expected

@@ -1,2 +1,4 @@
+| 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 |

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

c/misra/test/rules/RULE-5-4/header1.h

@@ -0,0 +1 @@
+#define REPEATED 11 // COMPLIANT

c/misra/test/rules/RULE-5-4/header2.h

@@ -0,0 +1 @@
+#define REPEATED 1 // COMPLIANT

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

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

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"

c/misra/test/rules/RULE-5-4/root2.c

@@ -0,0 +1,3 @@
+#include "conditional.h"
+
+#include "header4.h"

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 `<MACRO_NAME>` is not distinct from alternative definition of `<MACRO_NAME>` in `<relative_file_path>`.

cpp/common/src/codingstandards/cpp/Includes.qll

@@ -0,0 +1,37 @@
+/** 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`.
+ */
+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
+  )
+}
+
+/**
+ * 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 isConditionallyIncluded(_, i) and
+    result = i.getIncludedFile()
+  )
+}

cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll

@@ -40,3 +40,68 @@ class PreprocessorIfOrElif extends PreprocessorBranch {
     this instanceof PreprocessorElif
   }
 }
+
+/**
+ * Holds if the preprocessor directive `m` is located at `filepath` and `startline`.
+ */
+pragma[noinline]
+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 mutuallyExclusiveBranchDirectiveMacros(Macro firstMacro, Macro secondMacro) {
+  exists(PreprocessorBranchDirective b1, PreprocessorBranchDirective b2 |
+    isBranchDirectivePair(b1, b2, _, _, _) and
+    isMacroDefinedWithinBranch(b1, firstMacro) and
+    isMacroDefinedWithinBranch(b2, secondMacro)
+  )
+}