Merge pull request #770 from jbj/cfg-static-init-pr

C++: Add addresses to `Expr.isConstant`

Author: dave-bartolomeo

change-notes/1.20/analysis-cpp.md

@@ -26,4 +26,5 @@
 
 ## Changes to QL libraries
 
-There is a new `Namespace.isInline()` predicate, which holds if the namespace was declared as `inline namespace`.
+* There is a new `Namespace.isInline()` predicate, which holds if the namespace was declared as `inline namespace`.
+* The `Expr.isConstant()` predicate now also holds for _address constant expressions_, which are addresses that will be constant after the program has been linked. These address constants do not have a result for `Expr.getValue()`.

cpp/ql/src/Likely Bugs/Likely Typos/LogicalExprCouldBeSimplified.ql

@@ -43,7 +43,7 @@ string comparisonOnLiterals(ComparisonOperation op) {
   simple(op.getLeftOperand()) and
   simple(op.getRightOperand()) and
   not op.getAnOperand().isInMacroExpansion() and
-  if op.isConstant()
+  if exists(op.getValue())
   then result = "This comparison involves two literals and is always " + op.getValue() + "."
   else result = "This comparison involves two literals and should be simplified."
 }

cpp/ql/src/semmle/code/cpp/controlflow/internal/CFG.qll

@@ -461,38 +461,16 @@ private predicate skipInitializer(Initializer init) {
  */
 private predicate runtimeExprInStaticInitializer(Expr e) {
   inStaticInitializer(e) and
-  if
-    e instanceof AggregateLiteral
-    or
-    e instanceof PointerArithmeticOperation
-    or
-    // Extractor doesn't populate this specifier at the time of writing, so
-    // this case has not been tested. See CPP-314.
-    e.(FunctionCall).getTarget().hasSpecifier("constexpr")
+  if e instanceof AggregateLiteral
   then runtimeExprInStaticInitializer(e.getAChild())
-  else (
-    // Not constant
-    not e.isConstant() and
-    // Not a function address
-    not e instanceof FunctionAccess and
-    // Not a function address-of (same as above)
-    not e.(AddressOfExpr).getOperand() instanceof FunctionAccess and
-    // Not the address of a global variable
-    not exists(Variable v |
-      v.isStatic()
-      or
-      v instanceof GlobalOrNamespaceVariable
-    |
-      e.(AddressOfExpr).getOperand() = v.getAnAccess()
-    )
-  )
+  else not e.getFullyConverted().isConstant()
 }
 
 /** Holds if `e` is part of the initializer of a local static variable. */
 private predicate inStaticInitializer(Expr e) {
   exists(LocalVariable local |
     local.isStatic() and
-    e.(Node).getParentNode*() = local.getInitializer()
+    e.getParent+() = local.getInitializer()
   )
 }
 

cpp/ql/src/semmle/code/cpp/exprs/Expr.qll

@@ -2,6 +2,7 @@
 import semmle.code.cpp.Element
 private import semmle.code.cpp.Enclosing
 private import semmle.code.cpp.internal.ResolveClass
+private import semmle.code.cpp.internal.AddressConstantExpression
 
 /**
  * A C/C++ expression.
@@ -84,7 +85,12 @@ class Expr extends StmtParent, @expr {
   string getValueText() { exists(@value v | values(v,_,result) and valuebind(v,underlyingElement(this))) }
 
   /** Holds if this expression has a value that can be determined at compile time. */
-  predicate isConstant() { valuebind(_,underlyingElement(this)) }
+  cached
+  predicate isConstant() {
+    valuebind(_,underlyingElement(this))
+    or
+    addressConstantExpression(this)
+  }
 
   /**
    * Holds if this expression is side-effect free (conservative

cpp/ql/src/semmle/code/cpp/internal/AddressConstantExpression.qll

@@ -0,0 +1,184 @@
+private import cpp
+private import semmle.code.cpp.dataflow.EscapesTree
+
+predicate addressConstantExpression(Expr e) {
+  constantAddressPointer(e)
+  or
+  constantAddressReference(e)
+  or
+  // Special case for function pointers, where `fp == *fp`.
+  constantAddressLValue(e) and
+  e.getType() instanceof FunctionPointerType
+}
+
+/** Holds if `v` is a constexpr variable initialized to a constant address. */
+private predicate addressConstantVariable(Variable v) {
+  addressConstantExpression(v.getInitializer().getExpr().getFullyConverted()) and
+  // Here we should also require that `v` is constexpr, but we don't have that
+  // information in the db. See CPP-314. Instead, we require that the variable
+  // is never defined except in its initializer.
+  forall(Expr def | definition(v, def) | def = any(Initializer init).getExpr())
+}
+
+/**
+ * Holds if `lvalue` is an lvalue whose address is an _address constant
+ * expression_.
+ */
+private predicate constantAddressLValue(Expr lvalue) {
+  lvalue.(VariableAccess).getTarget() = any(Variable v |
+      v.(Variable).isStatic()
+      or
+      v instanceof GlobalOrNamespaceVariable
+    )
+  or
+  // There is no `Conversion` for the implicit conversion from a function type
+  // to a function _pointer_ type. Instead, the type of a `FunctionAccess`
+  // tells us how it's going to be used.
+  lvalue.(FunctionAccess).getType() instanceof RoutineType
+  or
+  // String literals have array types and undergo array-to-pointer conversion.
+  lvalue instanceof StringLiteral
+  or
+  // lvalue -> lvalue
+  exists(Expr prev |
+    constantAddressLValue(prev) and
+    lvalueToLvalueStep(prev, lvalue)
+  )
+  or
+  // pointer -> lvalue
+  exists(Expr prev |
+    constantAddressPointer(prev) and
+    pointerToLvalueStep(prev, lvalue)
+  )
+  or
+  // reference -> lvalue
+  exists(Expr prev |
+    constantAddressReference(prev) and
+    referenceToLvalueStep(prev, lvalue)
+  )
+}
+
+/** Holds if `pointer` is an _address constant expression_ of pointer type. */
+private predicate constantAddressPointer(Expr pointer) {
+  // There is no `Conversion` for the implicit conversion from a function type
+  // to a function _pointer_ type. Instead, the type of a `FunctionAccess`
+  // tells us how it's going to be used.
+  pointer.(FunctionAccess).getType() instanceof FunctionPointerType
+  or
+  addressConstantVariable(pointer.(VariableAccess).getTarget()) and
+  pointer.getType().getUnderlyingType() instanceof PointerType
+  or
+  // pointer -> pointer
+  exists(Expr prev |
+    constantAddressPointer(prev) and
+    pointerToPointerStep(prev, pointer)
+  )
+  or
+  // lvalue -> pointer
+  exists(Expr prev |
+    constantAddressLValue(prev) and
+    lvalueToPointerStep(prev, pointer)
+  )
+}
+
+/** Holds if `reference` is an _address constant expression_ of reference type. */
+private predicate constantAddressReference(Expr reference) {
+  addressConstantVariable(reference.(VariableAccess).getTarget()) and
+  reference.getType().getUnderlyingType() instanceof ReferenceType
+  or
+  addressConstantVariable(reference.(VariableAccess).getTarget()) and
+  reference.getType().getUnderlyingType() instanceof FunctionReferenceType // not a ReferenceType
+  or
+  // reference -> reference
+  exists(Expr prev |
+    constantAddressReference(prev) and
+    referenceToReferenceStep(prev, reference)
+  )
+  or
+  // lvalue -> reference
+  exists(Expr prev |
+    constantAddressLValue(prev) and
+    lvalueToReferenceStep(prev, reference)
+  )
+}
+
+private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
+  lvalueIn = lvalueOut.(DotFieldAccess).getQualifier().getFullyConverted()
+  or
+  lvalueIn.getConversion() = lvalueOut.(ParenthesisExpr)
+  or
+  // Special case for function pointers, where `fp == *fp`.
+  lvalueIn = lvalueOut.(PointerDereferenceExpr).getOperand().getFullyConverted() and
+  lvalueIn.getType() instanceof FunctionPointerType
+}
+
+private predicate pointerToLvalueStep(Expr pointerIn, Expr lvalueOut) {
+  lvalueOut = any(ArrayExpr ae |
+      pointerIn = ae.getArrayBase().getFullyConverted() and
+      hasConstantValue(ae.getArrayOffset().getFullyConverted())
+    )
+  or
+  pointerIn = lvalueOut.(PointerDereferenceExpr).getOperand().getFullyConverted()
+  or
+  pointerIn = lvalueOut.(PointerFieldAccess).getQualifier().getFullyConverted()
+}
+
+private predicate lvalueToPointerStep(Expr lvalueIn, Expr pointerOut) {
+  lvalueIn.getConversion() = pointerOut.(ArrayToPointerConversion)
+  or
+  lvalueIn = pointerOut.(AddressOfExpr).getOperand().getFullyConverted()
+}
+
+private predicate pointerToPointerStep(Expr pointerIn, Expr pointerOut) {
+  (
+    pointerOut instanceof PointerAddExpr
+    or
+    pointerOut instanceof PointerSubExpr
+  ) and
+  pointerIn = pointerOut.getAChild().getFullyConverted() and
+  pointerIn.getType().getUnspecifiedType() instanceof PointerType and
+  // The pointer arg won't be constant in the sense of `hasConstantValue`, so
+  // this will have to match the integer argument.
+  hasConstantValue(pointerOut.getAChild().getFullyConverted())
+  or
+  pointerIn = pointerOut.(UnaryPlusExpr).getOperand().getFullyConverted()
+  or
+  pointerIn.getConversion() = pointerOut.(Cast)
+  or
+  pointerIn.getConversion() = pointerOut.(ParenthesisExpr)
+  or
+  pointerOut = any(ConditionalExpr cond |
+      cond.getCondition().getFullyConverted().getValue().toInt() != 0 and
+      pointerIn = cond.getThen().getFullyConverted()
+      or
+      cond.getCondition().getFullyConverted().getValue().toInt() = 0 and
+      pointerIn = cond.getElse().getFullyConverted()
+    )
+  or
+  // The comma operator is allowed by C++17 but disallowed by C99. This
+  // disjunct is a compromise that's chosen for being easy to implement.
+  pointerOut = any(CommaExpr comma |
+      hasConstantValue(comma.getLeftOperand()) and
+      pointerIn = comma.getRightOperand().getFullyConverted()
+    )
+}
+
+private predicate lvalueToReferenceStep(Expr lvalueIn, Expr referenceOut) {
+  lvalueIn.getConversion() = referenceOut.(ReferenceToExpr)
+}
+
+private predicate referenceToLvalueStep(Expr referenceIn, Expr lvalueOut) {
+  // This probably cannot happen. It would require an expression to be
+  // converted to a reference and back again without an intermediate variable
+  // assignment.
+  referenceIn.getConversion() = lvalueOut.(ReferenceDereferenceExpr)
+}
+
+private predicate referenceToReferenceStep(Expr referenceIn, Expr referenceOut) {
+  referenceIn.getConversion() = referenceOut.(Cast)
+  or
+  referenceIn.getConversion() = referenceOut.(ParenthesisExpr)
+}
+
+/** Holds if `e` is constant according to the database. */
+private predicate hasConstantValue(Expr e) { valuebind(_, underlyingElement(e)) }

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -28,6 +28,13 @@ private Element getRealParent(Expr expr) {
   result.(Destructor).getADestruction() = expr
 }
 
+/**
+ * Holds if `expr` is a constant of a type that can be replaced directly with
+ * its value in the IR. This does not include address constants as we have no
+ * means to express those as QL values.
+ */
+predicate isIRConstant(Expr expr) { exists(expr.getValue()) }
+
 /**
  * Holds if `expr` and all of its descendants should be ignored for the purposes
  * of IR generation due to some property of `expr` itself. Unlike
@@ -42,7 +49,7 @@ private predicate ignoreExprAndDescendants(Expr expr) {
   getRealParent(expr) instanceof SwitchCase or
   // Ignore descendants of constant expressions, since we'll just substitute the
   // constant value.
-  getRealParent(expr).(Expr).isConstant() or
+  isIRConstant(getRealParent(expr)) or
   // The `DestructorCall` node for a `DestructorFieldDestruction` has a `FieldAccess`
   // node as its qualifier, but that `FieldAccess` does not have a child of its own.
   // We'll ignore that `FieldAccess`, and supply the receiver as part of the calling
@@ -125,7 +132,7 @@ private predicate translateStmt(Stmt stmt) {
  */
 private predicate isNativeCondition(Expr expr) {
   expr instanceof BinaryLogicalOperation and
-  not expr.isConstant()
+  not isIRConstant(expr)
 }
 
 /**
@@ -138,7 +145,7 @@ private predicate isFlexibleCondition(Expr expr) {
     expr instanceof NotExpr
   ) and
   usedAsCondition(expr) and
-  not expr.isConstant()
+  not isIRConstant(expr)
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

@@ -962,7 +962,7 @@ class TranslatedFunctionAccess extends TranslatedNonConstantExpr {
 abstract class TranslatedNonConstantExpr extends TranslatedCoreExpr, TTranslatedValueExpr {
   TranslatedNonConstantExpr() {
     this = TTranslatedValueExpr(expr) and
-    not expr.isConstant()
+    not isIRConstant(expr)
   }
 }
 
@@ -974,7 +974,7 @@ abstract class TranslatedNonConstantExpr extends TranslatedCoreExpr, TTranslated
 abstract class TranslatedConstantExpr extends TranslatedCoreExpr, TTranslatedValueExpr {
   TranslatedConstantExpr() {
     this = TTranslatedValueExpr(expr) and
-    expr.isConstant()
+    isIRConstant(expr)
   }
 
   override final Instruction getFirstInstruction() {