Merge branch 'main' into lcartey/rule-2-5-alt-uses

c/common/src/codingstandards/c/UndefinedBehavior.qll

@@ -1,4 +1,5 @@
 import cpp
+import codingstandards.cpp.Pointers
 import codingstandards.cpp.UndefinedBehavior
 
 /**
@@ -6,28 +7,38 @@ import codingstandards.cpp.UndefinedBehavior
  */
 abstract class CUndefinedBehavior extends UndefinedBehavior { }
 
+/**
+ * A function which has the signature - but not the name - of a main function.
+ */
 class C99MainFunction extends Function {
   C99MainFunction() {
     this.getNumberOfParameters() = 2 and
-    this.getType() instanceof IntType and
-    this.getParameter(0).getType() instanceof IntType and
-    this.getParameter(1).getType().(PointerType).getBaseType().(PointerType).getBaseType()
-      instanceof CharType
+    this.getType().getUnderlyingType() instanceof IntType and
+    this.getParameter(0).getType().getUnderlyingType() instanceof IntType and
+    this.getParameter(1)
+        .getType()
+        .getUnderlyingType()
+        .(UnspecifiedPointerOrArrayType)
+        .getBaseType()
+        .(UnspecifiedPointerOrArrayType)
+        .getBaseType() instanceof CharType
     or
     this.getNumberOfParameters() = 0 and
-    this.getType() instanceof VoidType
+    // Must be explicitly declared as `int main(void)`.
+    this.getADeclarationEntry().hasVoidParamList() and
+    this.getType().getUnderlyingType() instanceof IntType
   }
 }
 
 class CUndefinedMainDefinition extends CUndefinedBehavior, Function {
   CUndefinedMainDefinition() {
     // for testing purposes, we use the prefix ____codeql_coding_standards`
-    (this.getName() = "main" or this.getName().indexOf("____codeql_coding_standards") = 0) and
+    (this.getName() = "main" or this.getName().indexOf("____codeql_coding_standards_main") = 0) and
     not this instanceof C99MainFunction
   }
 
   override string getReason() {
     result =
-      "The behavior of the program is undefined because the main function is not defined according to the C standard."
+      "main function may trigger undefined behavior because it is not in one of the formats specified by the C standard."
   }
 }

c/misra/src/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.ql

@@ -18,4 +18,4 @@ import codingstandards.c.UndefinedBehavior
 
 from CUndefinedBehavior c
 where not isExcluded(c, Language3Package::occurrenceOfUndefinedBehaviorQuery())
-select c, "May result in undefined behavior."
+select c, c.getReason()

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/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql

@@ -18,29 +18,54 @@ import cpp
 import codingstandards.c.misra
 import codingstandards.cpp.Pointers
 import codingstandards.cpp.SideEffect
+import codingstandards.cpp.alertreporting.HoldsForAllCopies
 
-from Variable ptr, PointerOrArrayType type
+class NonConstPointerVariableCandidate extends Variable {
+  NonConstPointerVariableCandidate() {
+    // Ignore parameters in functions without bodies
+    (this instanceof Parameter implies exists(this.(Parameter).getFunction().getBlock())) and
+    // Ignore variables in functions that use ASM commands
+    not exists(AsmStmt a |
+      a.getEnclosingFunction() = this.(LocalScopeVariable).getFunction()
+      or
+      // In a type declared locally
+      this.(Field).getDeclaringType+().getEnclosingFunction() = a.getEnclosingFunction()
+    ) and
+    exists(PointerOrArrayType type |
+      // include only pointers which point to a const-qualified type
+      this.getType() = type and
+      not type.isDeeplyConstBelow()
+    ) and
+    // exclude pointers passed as arguments to functions which take a
+    // parameter that points to a non-const-qualified type
+    not exists(FunctionCall fc, int i |
+      fc.getArgument(i) = this.getAnAccess() and
+      not fc.getTarget().getParameter(i).getType().isDeeplyConstBelow()
+    ) and
+    // exclude any pointers which have their underlying data modified
+    not exists(VariableEffect effect |
+      effect.getTarget() = this and
+      // but not pointers that are only themselves modified
+      not effect.(AssignExpr).getLValue() = this.getAnAccess() and
+      not effect.(CrementOperation).getOperand() = this.getAnAccess()
+    ) and
+    // exclude pointers assigned to another pointer to a non-const-qualified type
+    not exists(Variable a |
+      a.getAnAssignedValue() = this.getAnAccess() and
+      not a.getType().(PointerOrArrayType).isDeeplyConstBelow()
+    )
+  }
+}
+
+/**
+ * Ensure that all copies of a variable are considered to be missing const qualification to avoid
+ * false positives where a variable is only used/modified in a single copy.
+ */
+class NonConstPointerVariable =
+  HoldsForAllCopies<NonConstPointerVariableCandidate, Variable>::LogicalResultElement;
+
+from NonConstPointerVariable ptr
 where
-  not isExcluded(ptr, Pointers1Package::pointerShouldPointToConstTypeWhenPossibleQuery()) and
-  // include only pointers which point to a const-qualified type
-  ptr.getType() = type and
-  not type.isDeeplyConstBelow() and
-  // exclude pointers passed as arguments to functions which take a
-  // parameter that points to a non-const-qualified type
-  not exists(FunctionCall fc, int i |
-    fc.getArgument(i) = ptr.getAnAccess() and
-    not fc.getTarget().getParameter(i).getType().isDeeplyConstBelow()
-  ) and
-  // exclude any pointers which have their underlying data modified
-  not exists(VariableEffect effect |
-    effect.getTarget() = ptr and
-    // but not pointers that are only themselves modified
-    not effect.(AssignExpr).getLValue() = effect.getAnAccess() and
-    not effect.(CrementOperation).getOperand() = effect.getAnAccess()
-  ) and
-  // exclude pointers assigned to another pointer to a non-const-qualified type
-  not exists(Variable a |
-    a.getAnAssignedValue() = ptr.getAnAccess() and
-    not a.getType().(PointerOrArrayType).isDeeplyConstBelow()
-  )
-select ptr, "$@ points to a non-const-qualified type.", ptr, ptr.getName()
+  not isExcluded(ptr.getAnElementInstance(),
+    Pointers1Package::pointerShouldPointToConstTypeWhenPossibleQuery())
+select ptr, "$@ points to a non-const-qualified type.", ptr, ptr.getAnElementInstance().getName()

c/misra/test/rules/RULE-1-3/OccurrenceOfUndefinedBehavior.expected

@@ -1,5 +1,8 @@
-| test.c:8:6:8:35 | ____codeql_coding_standards_m2 | May result in undefined behavior. |
-| test.c:11:5:11:34 | ____codeql_coding_standards_m3 | May result in undefined behavior. |
-| test.c:15:5:15:34 | ____codeql_coding_standards_m4 | May result in undefined behavior. |
-| test.c:19:5:19:34 | ____codeql_coding_standards_m5 | May result in undefined behavior. |
-| test.c:23:5:23:34 | ____codeql_coding_standards_m6 | May result in undefined behavior. |
+| test.c:4:6:4:38 | ____codeql_coding_standards_main1 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:8:5:8:37 | ____codeql_coding_standards_main2 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:27:5:27:37 | ____codeql_coding_standards_main6 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:32:6:32:38 | ____codeql_coding_standards_main7 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:36:5:36:37 | ____codeql_coding_standards_main8 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:40:5:40:37 | ____codeql_coding_standards_main9 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:44:5:44:38 | ____codeql_coding_standards_main10 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |
+| test.c:48:5:48:38 | ____codeql_coding_standards_main11 | main function may trigger undefined behavior because it is not in one of the formats specified by the C standard. |

c/misra/test/rules/RULE-1-3/test.c

@@ -1,25 +1,50 @@
-void main(void) { // COMPLIANT
+int main(void) { // COMPLIANT
 }
 
-int ____codeql_coding_standards_m1(int argc, char **argv) { // NON_COMPLIANT
+void ____codeql_coding_standards_main1(void) { // NON_COMPLIANT
   return 0;
 }
 
-void ____codeql_coding_standards_m2(char *argc, char **argv) { // NON_COMPLIANT
-}
-
-int ____codeql_coding_standards_m3(int argc, char *argv) { // NON_COMPLIANT
+int ____codeql_coding_standards_main2() { // NON_COMPLIANT
   return 0;
 }
 
-int ____codeql_coding_standards_m4() { // NON_COMPLIANT
+int ____codeql_coding_standards_main3(int argc, char **argv) { // COMPLIANT
   return 0;
 }
 
-int ____codeql_coding_standards_m5(int argc, int *argv) { // NON_COMPLIANT
+int ____codeql_coding_standards_main4(int argc, char argv[][]) { // COMPLIANT
   return 0;
 }
 
-int ____codeql_coding_standards_m6(int argc, int **argv) { // NON_COMPLIANT
+int ____codeql_coding_standards_main5(int argc, char *argv[]) { // COMPLIANT
+  return 0;
+}
+
+typedef int MY_INT;
+typedef char *MY_CHAR_PTR;
+
+int ____codeql_coding_standards_main6(MY_INT argc,
+                                      MY_CHAR_PTR argv[]) { // COMPLIANT
+  return 0;
+}
+
+void ____codeql_coding_standards_main7(char *argc,
+                                       char **argv) { // NON_COMPLIANT
+}
+
+int ____codeql_coding_standards_main8(int argc, char *argv) { // NON_COMPLIANT
+  return 0;
+}
+
+int ____codeql_coding_standards_main9() { // NON_COMPLIANT
+  return 0;
+}
+
+int ____codeql_coding_standards_main10(int argc, int *argv) { // NON_COMPLIANT
+  return 0;
+}
+
+int ____codeql_coding_standards_main11(int argc, int **argv) { // NON_COMPLIANT
   return 0;
 }

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"

c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected

@@ -12,3 +12,4 @@
 | test.c:66:23:66:24 | p1 | $@ points to a non-const-qualified type. | test.c:66:23:66:24 | p1 | p1 |
 | test.c:71:17:71:18 | p1 | $@ points to a non-const-qualified type. | test.c:71:17:71:18 | p1 | p1 |
 | test.c:75:15:75:16 | p1 | $@ points to a non-const-qualified type. | test.c:75:15:75:16 | p1 | p1 |
+| test.c:103:30:103:30 | s | $@ points to a non-const-qualified type. | test.c:103:30:103:30 | s | s |

c/misra/test/rules/RULE-8-13/test.c

@@ -75,4 +75,38 @@ char *f16(char *p1) { // NON_COMPLIANT
 int f17(char *p1) { // NON_COMPLIANT
   p1++;
   return 0;
+}
+
+#include <stdint.h>
+
+int16_t
+test_r(int16_t *value) { // COMPLIANT - ignored because of the use of ASM
+  int16_t result;
+  struct S {
+    int *x; // COMPLIANT - ignored because of the use of ASM
+    struct S2 {
+      int *y; // COMPLIANT - ignored because of the use of ASM
+    } s2;
+  };
+  __asm__("movb %bh (%eax)");
+  return result;
+}
+
+struct S {
+  int x;
+};
+
+void test_struct(struct S *s) { // COMPLIANT
+  s->x = 1;
+}
+
+void test_struct_2(struct S *s) { // NON_COMPLIANT - could be const
+  s = 0;
+}
+
+void test_no_body(int *p); // COMPLIANT - no body, so cannot evaluate whether it
+                           // should be const
+
+void increment(int *p) { // COMPLIANT
+  *p++ = 1;
 }

change_notes/2024-10-20-8-13-fixes.md

@@ -0,0 +1,7 @@
+ - `RULE-8-13` - `PointerShouldPointToConstTypeWhenPossible.ql`
+   - Exclude false positives where a variable occurs in a file compiled multiple times, but where it may only be const in some of those scenarios.
+   - Exclude results for local scope variables in functions that use assembly code, as CodeQL cannot determine the impact of the assembly.
+   - Exclude false positives when an assignment is made to a struct field.
+   - Exclude false positives where the object pointed to by the variable is modified using `*p++ = ...`.
+   - Exclude false positives for functions without bodies.
+ - Rules that rely on the determination of side-effects of an expression may change as a result of considering `*p++ = ...` as having a side-effect on `p`.

change_notes/2024-10-21-rule-1-3-main.md

@@ -0,0 +1,3 @@
+ - `RULE-1-3` - `OccurrenceOfUndefinedBehavior.ql`:
+   - Improve alert message to report the undefined behavior triggered.
+   - Address both false positives and false negatives in identifying standard compliant main methods. Previously, `void main()` was considered permitted and `int main(void)` banned. In addition, we now detect main methods as standard compliant if they use typedefs, and if arrays are used in the definition of `argv`.

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/Pointers.qll

@@ -6,7 +6,7 @@ import cpp
 import codingstandards.cpp.Type
 
 /**
- * A type that is a pointer or array type.
+ * A type that is a pointer or array type after stripping top-level specifiers.
  */
 class PointerOrArrayType extends DerivedType {
   PointerOrArrayType() {
@@ -15,6 +15,16 @@ class PointerOrArrayType extends DerivedType {
   }
 }
 
+/**
+ * A type that is a pointer or array type.
+ */
+class UnspecifiedPointerOrArrayType extends DerivedType {
+  UnspecifiedPointerOrArrayType() {
+    this instanceof PointerType or
+    this instanceof ArrayType
+  }
+}
+
 /**
  * An expression which performs pointer arithmetic
  */

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)
+  )
+}

cpp/common/src/codingstandards/cpp/SideEffect.qll

@@ -190,6 +190,8 @@ Expr getAnEffect(Expr base) {
   or
   exists(PointerDereferenceExpr e | e.getOperand() = base | result = getAnEffect(e))
   or
+  exists(CrementOperation c | c.getOperand() = base | result = getAnEffect(c))
+  or
   // local alias analysis, assume alias when data flows to derived type (pointer/reference)
   // auto ptr = &base;
   exists(VariableAccess va, AddressOfExpr addressOf |

cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll

@@ -85,11 +85,9 @@ module HoldsForAllCopies<CandidateElementSig CandidateElement, ElementSetSig Ele
       string filepath, int startline, int startcolumn, int endline, int endcolumn
     ) {
       exists(CandidateElement s |
-        // Only consider candidates where we can match up the location
-        isNotWithinMacroExpansion(s) and
         hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) and
         // All relevant elements that occur at the same location are candidates
-        forex(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
+        forall(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
           relevantElement instanceof CandidateElement
         )
       )