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>`.