Merge pull request #884 from github/michaelrfairhurst/address-compiler-compatibility-issue-misra-c-2023

First pass at addressing cross-compiler compatibility in MISRA 2023.

Author: MichaelRFairhurst

c/common/src/codingstandards/c/TgMath.qll

@@ -1,21 +1,38 @@
 import cpp
 
-private string getATgMathMacroName(boolean allowComplex) {
+private string getATgMathMacroName(boolean allowComplex, int numberOfParameters) {
   allowComplex = true and
+  numberOfParameters = 1 and
   result =
     [
       "acos", "acosh", "asin", "asinh", "atan", "atanh", "carg", "cimag", "conj", "cos", "cosh",
-      "cproj", "creal", "exp", "fabs", "log", "pow", "sin", "sinh", "sqrt", "tan", "tanh"
+      "cproj", "creal", "exp", "fabs", "log", "sin", "sinh", "sqrt", "tan", "tanh"
+    ]
+  or
+  allowComplex = true and
+  numberOfParameters = 2 and
+  result = "pow"
+  or
+  allowComplex = false and
+  numberOfParameters = 1 and
+  result =
+    [
+      "cbrt", "ceil", "erf", "erfc", "exp2", "expm1", "floor", "ilogb", "lgamma", "llrint",
+      "llround", "log10", "log1p", "log2", "logb", "lrint", "lround", "nearbyint", "rint", "round",
+      "tgamma", "trunc",
     ]
   or
   allowComplex = false and
+  numberOfParameters = 2 and
   result =
     [
-      "atan2", "cbrt", "ceil", "copysign", "erf", "erfc", "exp2", "expm1", "fdim", "floor", "fma",
-      "fmax", "fmin", "fmod", "frexp", "hypot", "ilogb", "ldexp", "lgamma", "llrint", "llround",
-      "log10", "log1p", "log2", "logb", "lrint", "lround", "nearbyint", "nextafter", "nexttoward",
-      "remainder", "remquo", "rint", "round", "scalbn", "scalbln", "tgamma", "trunc",
+      "atan2", "copysign", "fdim", "fmax", "fmin", "fmod", "frexp", "hypot", "ldexp", "nextafter",
+      "nexttoward", "remainder", "scalbn", "scalbln"
     ]
+  or
+  allowComplex = false and
+  numberOfParameters = 3 and
+  result = ["fma", "remquo"]
 }
 
 private predicate hasOutputArgument(string macroName, int index) {
@@ -27,19 +44,41 @@ private predicate hasOutputArgument(string macroName, int index) {
 class TgMathInvocation extends MacroInvocation {
   Call call;
   boolean allowComplex;
+  int numberOfParameters;
 
   TgMathInvocation() {
-    this.getMacro().getName() = getATgMathMacroName(allowComplex) and
+    this.getMacro().getName() = getATgMathMacroName(allowComplex, numberOfParameters) and
     call = getBestCallInExpansion(this)
   }
 
+  /** Account for extra parameters added by gcc */
+  private int getParameterOffset() {
+    // Gcc calls look something like: `__builtin_tgmath(cosf, cosd, cosl, arg)`, in this example
+    // there is a parameter offset of 3, so `getOperandArgument(0)` is equivalent to
+    // `call.getArgument(3)`.
+    result = call.getNumberOfArguments() - numberOfParameters
+  }
+
   Expr getOperandArgument(int i) {
-    result = call.getArgument(i) and
-    not hasOutputArgument(call.getTarget().getName(), i)
+    i >= 0 and
+    result = call.getArgument(i + getParameterOffset()) and
+    //i in [0..numberOfParameters - 1] and
+    not hasOutputArgument(getMacro().getName(), i)
+  }
+
+  /** Get all explicit conversions, except those added by clang in the macro body */
+  Expr getExplicitlyConvertedOperandArgument(int i) {
+    exists(Expr explicitConv |
+      explicitConv = getOperandArgument(i).getExplicitlyConverted() and
+      // clang explicitly casts most arguments, but not some integer arguments such as in `scalbn`.
+      if call.getTarget().getName().matches("__tg_%") and explicitConv instanceof Conversion
+      then result = explicitConv.(Conversion).getExpr()
+      else result = explicitConv
+    )
   }
 
   int getNumberOfOperandArguments() {
-    result = call.getNumberOfArguments() - count(int i | hasOutputArgument(getMacroName(), i))
+    result = numberOfParameters - count(int i | hasOutputArgument(getMacroName(), i))
   }
 
   Expr getAnOperandArgument() { result = getOperandArgument(_) }

c/common/test/rules/functionnoreturnattributecondition/test.c

@@ -77,7 +77,7 @@ _Noreturn void test_noreturn_f10(int i) { // COMPLIANT
   case 4:
     thrd_exit(0);
     break;
-  default:
+  default:;
     jmp_buf jb;
     longjmp(jb, 0);
   }

c/misra/src/rules/RULE-13-2/UnsequencedAtomicReads.ql

@@ -17,15 +17,16 @@ import semmle.code.cpp.dataflow.TaintTracking
 import codingstandards.c.misra
 import codingstandards.c.Ordering
 import codingstandards.c.orderofevaluation.VariableAccessOrdering
+import codingstandards.cpp.StdFunctionOrMacro
 
 class AtomicAccessInFullExpressionOrdering extends Ordering::Configuration {
   AtomicAccessInFullExpressionOrdering() { this = "AtomicAccessInFullExpressionOrdering" }
 
   override predicate isCandidate(Expr e1, Expr e2) {
     exists(AtomicVariableAccess a, AtomicVariableAccess b, FullExpr e | a = e1 and b = e2 |
       a.getTarget() = b.getTarget() and
-      a.(ConstituentExpr).getFullExpr() = e and
-      b.(ConstituentExpr).getFullExpr() = e and
+      a.getARead().(ConstituentExpr).getFullExpr() = e and
+      b.getARead().(ConstituentExpr).getFullExpr() = e and
       not a = b
     )
   }
@@ -39,21 +40,40 @@ class AtomicAccessInFullExpressionOrdering extends Ordering::Configuration {
 class AtomicVariableAccess extends VariableAccess {
   AtomicVariableAccess() { getTarget().getType().hasSpecifier("atomic") }
 
-  /* Get the `atomic_<read|write>()` call this VarAccess occurs in. */
-  FunctionCall getAtomicFunctionCall() {
-    exists(AddressOfExpr addrParent, FunctionCall fc |
-      fc.getTarget().getName().matches("__c11_atomic%") and
+  /* Get the `atomic_load()` call this VarAccess occurs in. */
+  Expr getAtomicFunctionRead() {
+    exists(AddressOfExpr addrParent, AtomicReadOrWriteCall fc |
+      fc.getName().matches("atomic_load%") and
+      // StdFunctionOrMacro arguments are not necessarily reliable, so we look for any AddressOfExpr
+      // that is an argument to a call to `atomic_load`.
       addrParent = fc.getArgument(0) and
       addrParent.getAnOperand() = this and
-      result = fc
+      result = fc.getExpr()
+    )
+  }
+
+  /* Get the `atomic_store()` call this VarAccess occurs in. */
+  Expr getAtomicFunctionWrite(Expr storedValue) {
+    exists(AddressOfExpr addrParent, AtomicReadOrWriteCall fc |
+      addrParent = fc.getArgument(0) and
+      addrParent.getAnOperand() = this and
+      result = fc.getExpr() and
+      (
+        fc.getName().matches(["%store%", "%exchange%", "%fetch_%"]) and
+        not fc.getName().matches("%compare%") and
+        storedValue = fc.getArgument(1)
+        or
+        fc.getName().matches(["%compare%"]) and
+        storedValue = fc.getArgument(2)
+      )
     )
   }
 
   /**
    * Gets an assigned expr, either in the form `x = <result>` or `atomic_store(&x, <result>)`.
    */
   Expr getAnAssignedExpr() {
-    result = getAtomicFunctionCall().getArgument(1)
+    exists(getAtomicFunctionWrite(result))
     or
     exists(AssignExpr assign |
       assign.getLValue() = this and
@@ -65,19 +85,25 @@ class AtomicVariableAccess extends VariableAccess {
    * Gets the expression holding this variable access, either in the form `x` or `atomic_read(&x)`.
    */
   Expr getARead() {
-    result = getAtomicFunctionCall()
+    result = getAtomicFunctionRead()
     or
     result = this
   }
 }
 
 from
   AtomicAccessInFullExpressionOrdering config, FullExpr e, Variable v, AtomicVariableAccess va1,
-  AtomicVariableAccess va2
+  AtomicVariableAccess va2, Expr va1Read, Expr va2Read
 where
   not isExcluded(e, SideEffects3Package::unsequencedAtomicReadsQuery()) and
-  e = va1.(ConstituentExpr).getFullExpr() and
-  config.isUnsequenced(va1, va2) and
+  va1Read = va1.getARead() and
+  va2Read = va2.getARead() and
+  e = va1Read.(ConstituentExpr).getFullExpr() and
+  // Careful here. The `VariableAccess` in a pair of atomic function calls may not be unsequenced,
+  // for instance in gcc where atomic functions expand to StmtExprs, which have clear sequences.
+  // In this case, the result of `getARead()` for a pair of atomic function calls may be
+  // unsequenced even though the `VariableAccess`es within those calls are not.
+  config.isUnsequenced(va1Read, va2Read) and
   v = va1.getTarget() and
   v = va2.getTarget() and
   // Exclude cases where the variable is assigned a value tainted by the other variable access.

c/misra/src/rules/RULE-21-22/TgMathArgumentWithInvalidEssentialType.ql

@@ -34,14 +34,19 @@ string getAllowedTypesString(TgMathInvocation call) {
   else result = "essentially signed, unsigned, or real floating type"
 }
 
-from TgMathInvocation call, Expr arg, int argIndex, Type type, EssentialTypeCategory category
+from
+  TgMathInvocation call, Expr convertedArg, Expr unconverted, int argIndex, Type type,
+  EssentialTypeCategory category
 where
   not isExcluded(call, EssentialTypes2Package::tgMathArgumentWithInvalidEssentialTypeQuery()) and
-  arg = call.getOperandArgument(argIndex) and
-  type = getEssentialType(arg) and
+  // We must handle conversions specially, as clang inserts casts in the macro body we want to ignore.
+  convertedArg = call.getExplicitlyConvertedOperandArgument(argIndex) and
+  unconverted = convertedArg.getUnconverted() and
+  // Do not use `convertedArg.getEssentialType()`, as that is affected by clang's casts in the macro body.
+  type = getEssentialTypeBeforeConversions(convertedArg) and
   category = getEssentialTypeCategory(type) and
   not category = getAnAllowedEssentialTypeCategory(call)
-select arg,
+select unconverted,
   "Argument " + (argIndex + 1) + " provided to type-generic macro '" + call.getMacroName() +
     "' has " + category.toString().toLowerCase() + ", which is not " + getAllowedTypesString(call) +
     "."

c/misra/src/rules/RULE-21-23/TgMathArgumentsWithDifferingStandardType.ql

@@ -58,15 +58,13 @@ Type canonicalize(Type type) {
   else result = type
 }
 
-Type getEffectiveStandardType(Expr e) {
-  result = canonicalize(getPromotedType(e.getExplicitlyConverted()))
-}
+Type getEffectiveStandardType(Expr e) { result = canonicalize(getPromotedType(e)) }
 
 from TgMathInvocation call, Type firstType
 where
   not isExcluded(call, EssentialTypes2Package::tgMathArgumentsWithDifferingStandardTypeQuery()) and
-  firstType = getEffectiveStandardType(call.getAnOperandArgument()) and
-  not forall(Expr arg | arg = call.getAnOperandArgument() |
+  firstType = getEffectiveStandardType(call.getExplicitlyConvertedOperandArgument(0)) and
+  not forall(Expr arg | arg = call.getExplicitlyConvertedOperandArgument(_) |
     firstType = getEffectiveStandardType(arg)
   )
 select call,

c/misra/src/rules/RULE-21-25/InvalidMemoryOrderArgument.ql

@@ -14,11 +14,23 @@
 
 import cpp
 import codingstandards.c.misra
+import codingstandards.cpp.StdFunctionOrMacro
 import semmle.code.cpp.dataflow.new.DataFlow
 
+class MemoryOrderEnum extends Enum {
+  MemoryOrderEnum() {
+    this.hasGlobalOrStdName("memory_order")
+    or
+    exists(TypedefType t |
+      t.getName() = "memory_order" and
+      t.getBaseType() = this
+    )
+  }
+}
+
 /* A member of the set of memory orders defined in the `memory_order` enum */
 class MemoryOrder extends EnumConstant {
-  MemoryOrder() { getDeclaringEnum().getName() = "memory_order" }
+  MemoryOrder() { getDeclaringEnum() instanceof MemoryOrderEnum }
 
   int getIntValue() { result = getValue().toInt() }
 }
@@ -49,59 +61,6 @@ class MemoryOrderConstantExpr extends Expr {
   string getMemoryOrderString() { result = ord.getName() }
 }
 
-/**
- * A `stdatomic.h` function which accepts a `memory_order` value as a parameter.
- */
-class MemoryOrderedStdAtomicFunction extends Function {
-  int orderParamIdx;
-
-  MemoryOrderedStdAtomicFunction() {
-    exists(int baseParamIdx, int baseParams, string prefix, string regex, string basename |
-      regex = "__(c11_)?atomic_([a-z_]+)" and
-      prefix = getName().regexpCapture(regex, 1) and
-      basename = "atomic_" + getName().regexpCapture(regex, 2) + ["", "_explicit"] and
-      (
-        basename in ["atomic_thread_fence", "atomic_signal_fence"] and
-        baseParamIdx = 0 and
-        baseParams = 1
-        or
-        basename in ["atomic_load", "atomic_flag_clear", "atomic_flag_test_and_set"] and
-        baseParamIdx = 1 and
-        baseParams = 2
-        or
-        basename in [
-            "atomic_store", "atomic_fetch_" + ["add", "sub", "or", "xor", "and"], "atomic_exchange"
-          ] and
-        baseParamIdx = 2 and
-        baseParams = 3
-        or
-        basename in ["atomic_compare_exchange_" + ["strong", "weak"]] and
-        baseParamIdx = [3, 4] and
-        baseParams = 5
-      ) and
-      (
-        // GCC case, may have one or two inserted parameters, e.g.:
-        // __atomic_load(8, &repr->a, &desired, order)
-        // or
-        // __atomic_load_8(&repr->a, &desired, order)
-        prefix = "" and
-        exists(int extraParams |
-          extraParams = getNumberOfParameters() - baseParams and
-          extraParams >= 0 and
-          orderParamIdx = baseParamIdx + extraParams
-        )
-        or
-        // Clang case, no inserted parameters:
-        // __c11_atomic_load(object, order)
-        prefix = "c11_" and
-        orderParamIdx = baseParamIdx
-      )
-    )
-  }
-
-  int getOrderParameterIdx() { result = orderParamIdx }
-}
-
 module MemoryOrderFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) {
     // Direct usage of memory order constant
@@ -118,10 +77,7 @@ module MemoryOrderFlowConfig implements DataFlow::ConfigSig {
   }
 
   predicate isSink(DataFlow::Node node) {
-    exists(FunctionCall fc |
-      node.asExpr() =
-        fc.getArgument(fc.getTarget().(MemoryOrderedStdAtomicFunction).getOrderParameterIdx())
-    )
+    exists(AtomicallySequencedCall call | call.getAMemoryOrderArgument() = node.asExpr())
   }
 }
 
@@ -140,7 +96,7 @@ string describeMemoryOrderNode(DataFlow::Node node) {
 }
 
 from
-  Expr argument, Function function, string value, MemoryOrderFlow::PathNode source,
+  Expr argument, AtomicallySequencedCall function, string value, MemoryOrderFlow::PathNode source,
   MemoryOrderFlow::PathNode sink
 where
   not isExcluded(argument, Concurrency6Package::invalidMemoryOrderArgumentQuery()) and
@@ -149,6 +105,6 @@ where
   value = describeMemoryOrderNode(source.getNode()) and
   // Double check that we didn't find flow from something equivalent to the allowed value.
   not value = any(AllowedMemoryOrder e).getName() and
-  function.getACallToThisFunction().getAnArgument() = argument
+  function.getAMemoryOrderArgument() = argument
 select argument, source, sink, "Invalid memory order '$@' in call to function '$@'.", value, value,
   function, function.getName()

c/misra/src/rules/RULE-9-7/UninitializedAtomicObject.ql

@@ -31,7 +31,11 @@ class ThreadSpawningFunction extends Function {
 }
 
 class AtomicInitAddressOfExpr extends AddressOfExpr {
-  AtomicInitAddressOfExpr() { exists(AtomicInitCall c | this = c.getArgument(0)) }
+  AtomicInitAddressOfExpr() {
+    // StdFunctionOrMacro arguments are not necessarily reliable, so we look for any AddressOfExpr
+    // that is an argument to a call to `atomic_init`.
+    exists(AtomicInitCall c | this = c.getAnArgument())
+  }
 }
 
 ControlFlowNode getARequiredInitializationPoint(LocalScopeVariable v) {

c/misra/test/rules/DIR-5-1/test.c

@@ -99,7 +99,7 @@ void many_thread13_calls_nonreentrant_funcs(void *p) {
   wcsrtombs(NULL, NULL, 0, NULL); // NON-COMPLIANT
 }
 
-void main() {
+int main(int argc, char *argv[]) {
   thrd_t single_thread1;
   thrd_t many_thread2;
   thrd_t single_thread3;

c/misra/test/rules/DIR-5-3/test.c

@@ -14,7 +14,7 @@ void func_called_from_main(void);
 void make_threads_called_from_func_called_from_main(void);
 void make_threads_called_from_main_pthread_thrd(void);
 
-void main() {
+int main(int argc, char *argv[]) {
   thrd_create(&g1, &thrd_func, NULL);             // COMPLIANT
   pthread_create(&g2, NULL, &pthread_func, NULL); // COMPLIANT
 

c/misra/test/rules/RULE-1-5/UseOfObsoleteMacroAtomicVarInit.expected.gcc

@@ -0,0 +1 @@
+| test.c:29:18:29:36 | ATOMIC_VAR_INIT(VALUE) | Usage of macro ATOMIC_VAR_INIT() is declared obscelescent in C18, and discouraged in earlier C versions. |