Treat CallSideEffect and InitializeDynamicAllocation the same as other side effects during IR generation

This commit moves the IR generation for the `CallSideEffect` and `InitializeDynamicAllocation` side effect instruction into their own subclasses of `TranslatedSideEffect`. Previously, they were embeddded in `TranslatedCall` and `TranslatedAllocationSideEffects`. There are no diffs in the generated IR. This just makes the implementation of all side effect generation be consistent.

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/SideEffects.qll

@@ -112,28 +112,41 @@ private predicate hasDefaultSideEffect(Call call, ParameterIndex i, boolean buff
 }
 
 /**
- * Gets the `SideEffectFunction` called by the specified expression.class, if known.
+ * A `Call` or `NewOrNewArrayExpr`.
  *
- * Thie will return a result only for `Call`, in which case it returns the target of the call, or
- * for `NewExpr` and `NewArrayExpr`, in which case it returns the allocator function called by the
- * expression.
+ * Both kinds of expression invoke a function as part of their evaluation. This class provides a
+ * way to treat both kinds of function similarly, and to get the invoked `Function`.
  */
-private SideEffectFunction getCallOrAllocationSideEffectFunction(Expr expr) {
-  result = expr.(Call).getTarget()
-  or
-  result = expr.(NewOrNewArrayExpr).getAllocator()
+class CallOrAllocationExpr extends Expr {
+  CallOrAllocationExpr() {
+    this instanceof Call
+    or
+    this instanceof NewOrNewArrayExpr
+  }
+
+  /** Gets the `Function` invoked by this expression, if known. */
+  final Function getTarget() {
+    result = this.(Call).getTarget()
+    or
+    result = this.(NewOrNewArrayExpr).getAllocator()
+  }
 }
 
 /**
  * Returns the side effect opcode, if any, that represents any side effects not specifically modeled
  * by an argument side effect.
  */
-Opcode getCallSideEffectOpcode(Expr expr) {
-  if not getCallOrAllocationSideEffectFunction(expr).hasOnlySpecificWriteSideEffects()
-  then result instanceof Opcode::CallSideEffect
-  else (
-    not getCallOrAllocationSideEffectFunction(expr).hasOnlySpecificReadSideEffects() and
-    result instanceof Opcode::CallReadSideEffect
+Opcode getCallSideEffectOpcode(CallOrAllocationExpr expr) {
+  not exists(expr.getTarget().(SideEffectFunction)) and result instanceof Opcode::CallSideEffect
+  or
+  exists(SideEffectFunction sideEffectFunction |
+    sideEffectFunction = expr.getTarget() and
+    if not sideEffectFunction.hasOnlySpecificWriteSideEffects()
+    then result instanceof Opcode::CallSideEffect
+    else (
+      not sideEffectFunction.hasOnlySpecificReadSideEffects() and
+      result instanceof Opcode::CallReadSideEffect
+    )
   )
 }
 

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

@@ -49,16 +49,6 @@ abstract class TranslatedCall extends TranslatedExpr {
     tag = CallTag() and
     opcode instanceof Opcode::Call and
     resultType = getTypeForPRValue(getCallResultType())
-    or
-    tag = CallSideEffectTag() and
-    opcode = getCallSideEffectOpcode(expr) and
-    (
-      opcode instanceof Opcode::CallSideEffect and
-      resultType = getUnknownType()
-      or
-      opcode instanceof Opcode::CallReadSideEffect and
-      resultType = getVoidType()
-    )
   }
 
   override Instruction getChildSuccessor(TranslatedElement child) {
@@ -81,25 +71,8 @@ abstract class TranslatedCall extends TranslatedExpr {
 
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     kind instanceof GotoEdge and
-    (
-      (
-        tag = CallTag() and
-        if hasSideEffect()
-        then result = getInstruction(CallSideEffectTag())
-        else
-          if hasPreciseSideEffect()
-          then result = getSideEffects().getFirstInstruction()
-          else result = getParent().getChildSuccessor(this)
-      )
-      or
-      (
-        hasSideEffect() and
-        tag = CallSideEffectTag() and
-        if hasPreciseSideEffect()
-        then result = getSideEffects().getFirstInstruction()
-        else result = getParent().getChildSuccessor(this)
-      )
-    )
+    tag = CallTag() and
+    result = getSideEffects().getFirstInstruction()
   }
 
   override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
@@ -118,15 +91,6 @@ abstract class TranslatedCall extends TranslatedExpr {
     )
   }
 
-  final override CppType getInstructionMemoryOperandType(
-    InstructionTag tag, TypedOperandTag operandTag
-  ) {
-    tag = CallSideEffectTag() and
-    hasSideEffect() and
-    operandTag instanceof SideEffectOperandTag and
-    result = getUnknownType()
-  }
-
   final override Instruction getResult() { result = getInstruction(CallTag()) }
 
   /**
@@ -197,27 +161,25 @@ abstract class TranslatedCall extends TranslatedExpr {
    */
   abstract predicate hasArguments();
 
-  final private predicate hasSideEffect() { exists(getCallSideEffectOpcode(expr)) }
-
-  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
-    hasSideEffect() and
-    tag = CallSideEffectTag() and
-    result = getResult()
-  }
-
-  predicate hasPreciseSideEffect() { exists(getSideEffects()) }
-
   final TranslatedSideEffects getSideEffects() { result.getExpr() = expr }
 }
 
+/**
+ * The IR translation of the side effects of the parent `TranslatedElement`.
+ *
+ * This object does not itself generate the side effect instructions. Instead, its children provide
+ * the actual side effects, with this object acting as a placeholder so the parent only needs to
+ * insert this one element at the point where all the side effects are supposed to occur.
+ */
 abstract class TranslatedSideEffects extends TranslatedElement {
+  /** Gets the expression whose side effects are being modeled. */
   abstract Expr getExpr();
 
   final override Locatable getAST() { result = getExpr() }
 
   final override Function getFunction() { result = getExpr().getEnclosingFunction() }
 
-  override TranslatedElement getChild(int i) {
+  final override TranslatedElement getChild(int i) {
     result =
       rank[i + 1](TranslatedSideEffect tse, int group, int indexInGroup |
         tse.getPrimaryExpr() = getExpr() and
@@ -236,12 +198,21 @@ abstract class TranslatedSideEffects extends TranslatedElement {
     )
   }
 
-  /**
-   * Gets the `TranslatedFunction` containing this expression.
-   */
-  final TranslatedFunction getEnclosingFunction() {
-    result = getTranslatedFunction(getExpr().getEnclosingFunction())
+  final override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
+    none()
   }
+
+  final override Instruction getFirstInstruction() {
+    result = getChild(0).getFirstInstruction()
+    or
+    // Some functions, like `std::move()`, have no side effects whatsoever.
+    not exists(getChild(0)) and result = getParent().getChildSuccessor(this)
+  }
+
+  final override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
+
+  /** Gets the primary instruction to be associated with each side effect instruction. */
+  abstract Instruction getPrimaryInstruction();
 }
 
 /**
@@ -345,64 +316,24 @@ class TranslatedStructorCall extends TranslatedFunctionCall {
   override predicate hasQualifier() { any() }
 }
 
-class TranslatedAllocationSideEffects extends TranslatedSideEffects,
-  TTranslatedAllocationSideEffects {
-  AllocationExpr expr;
-
-  TranslatedAllocationSideEffects() { this = TTranslatedAllocationSideEffects(expr) }
-
-  final override AllocationExpr getExpr() { result = expr }
-
-  override string toString() { result = "(allocation side effects for " + expr.toString() + ")" }
-
-  override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
-
-  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
-    opcode instanceof Opcode::InitializeDynamicAllocation and
-    tag = OnlyInstructionTag() and
-    type = getUnknownType()
-  }
-
-  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
-    tag = OnlyInstructionTag() and
-    kind = EdgeKind::gotoEdge() and
-    if exists(getChild(0))
-    then result = getChild(0).getFirstInstruction()
-    else result = getParent().getChildSuccessor(this)
-  }
-
-  override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
-    tag = OnlyInstructionTag() and
-    operandTag = addressOperand() and
-    result = getPrimaryInstructionForSideEffect(OnlyInstructionTag())
-  }
-
-  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
-    tag = OnlyInstructionTag() and
-    if expr instanceof NewOrNewArrayExpr
-    then result = getTranslatedAllocatorCall(expr).getInstruction(CallTag())
-    else result = getTranslatedCallInstruction(expr)
-  }
-}
-
+/**
+ * The IR translation of the side effects of a function call, including the implicit allocator
+ * call in a `new` or `new[]` expression.
+ */
 class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSideEffects {
-  Call expr;
+  Expr expr;
 
   TranslatedCallSideEffects() { this = TTranslatedCallSideEffects(expr) }
 
-  override string toString() { result = "(side effects  for " + expr.toString() + ")" }
+  final override string toString() { result = "(side effects  for " + expr.toString() + ")" }
 
-  override Call getExpr() { result = expr }
+  final override Expr getExpr() { result = expr }
 
-  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) { none() }
-
-  override Instruction getFirstInstruction() { result = getChild(0).getFirstInstruction() }
-
-  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
-
-  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
-    tag = OnlyInstructionTag() and
-    result = getTranslatedCallInstruction(expr)
+  final override Instruction getPrimaryInstruction() {
+    expr instanceof Call and result = getTranslatedCallInstruction(expr)
+    or
+    expr instanceof NewOrNewArrayExpr and
+    result = getTranslatedAllocatorCall(expr).getInstruction(CallTag())
   }
 }
 
@@ -441,6 +372,13 @@ abstract class TranslatedSideEffect extends TranslatedElement {
     kind instanceof GotoEdge
   }
 
+  final override Function getFunction() { result = getParent().getFunction() }
+
+  final override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = getParent().(TranslatedSideEffects).getPrimaryInstruction()
+  }
+
   /**
    * Gets the expression that caused this side effect.
    *
@@ -490,11 +428,6 @@ abstract class TranslatedArgumentSideEffect extends TranslatedSideEffect {
     if isWrite() then group = argumentWriteGroup() else group = argumentReadGroup()
   }
 
-  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
-    tag = OnlyInstructionTag() and
-    result = getTranslatedCallInstruction(call)
-  }
-
   final override int getInstructionIndex(InstructionTag tag) {
     tag = OnlyInstructionTag() and
     result = index
@@ -507,11 +440,6 @@ abstract class TranslatedArgumentSideEffect extends TranslatedSideEffect {
     result = getTranslatedFunction(call.getEnclosingFunction())
   }
 
-  /**
-   * Gets the `Function` containing this expression.
-   */
-  final override Function getFunction() { result = call.getEnclosingFunction() }
-
   final override predicate sideEffectInstruction(Opcode opcode, CppType type) {
     opcode = sideEffectOpcode and
     (
@@ -638,3 +566,70 @@ class TranslatedStructorQualifierSideEffect extends TranslatedArgumentSideEffect
     )
   }
 }
+
+/** The IR translation of the non-argument-specific side effect of a call. */
+class TranslatedCallSideEffect extends TranslatedSideEffect, TTranslatedCallSideEffect {
+  Expr expr;
+  SideEffectOpcode sideEffectOpcode;
+
+  TranslatedCallSideEffect() { this = TTranslatedCallSideEffect(expr, sideEffectOpcode) }
+
+  override Locatable getAST() { result = expr }
+
+  override Expr getPrimaryExpr() { result = expr }
+
+  override predicate sortOrder(int group, int indexInGroup) {
+    group = callSideEffectGroup() and indexInGroup = 0
+  }
+
+  override string toString() { result = "(call side effect for '" + expr.toString() + "')" }
+
+  override predicate sideEffectInstruction(Opcode opcode, CppType type) {
+    opcode = sideEffectOpcode and
+    (
+      opcode instanceof Opcode::CallSideEffect and
+      type = getUnknownType()
+      or
+      opcode instanceof Opcode::CallReadSideEffect and
+      type = getVoidType()
+    )
+  }
+
+  override CppType getInstructionMemoryOperandType(InstructionTag tag, TypedOperandTag operandTag) {
+    tag instanceof OnlyInstructionTag and
+    operandTag instanceof SideEffectOperandTag and
+    result = getUnknownType()
+  }
+}
+
+/**
+ * The IR translation of the allocation side effect of a call to a memory allocation function.
+ *
+ * This side effect provides a definition for the newly-allocated memory.
+ */
+class TranslatedAllocationSideEffect extends TranslatedSideEffect, TTranslatedAllocationSideEffect {
+  AllocationExpr expr;
+
+  TranslatedAllocationSideEffect() { this = TTranslatedAllocationSideEffect(expr) }
+
+  override Locatable getAST() { result = expr }
+
+  override Expr getPrimaryExpr() { result = expr }
+
+  override predicate sortOrder(int group, int indexInGroup) {
+    group = initializeAllocationGroup() and indexInGroup = 0
+  }
+
+  override string toString() { result = "(allocation side effect for '" + expr.toString() + "')" }
+
+  override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
+    tag = OnlyInstructionTag() and
+    operandTag = addressOperand() and
+    result = getPrimaryInstructionForSideEffect(OnlyInstructionTag())
+  }
+
+  override predicate sideEffectInstruction(Opcode opcode, CppType type) {
+    opcode instanceof Opcode::InitializeDynamicAllocation and
+    type = getUnknownType()
+  }
+}

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

@@ -135,6 +135,20 @@ private predicate ignoreExpr(Expr expr) {
   ignoreExprAndDescendants(expr)
 }
 
+/**
+ * Holds if the side effects of `expr` should be ignoredf for the purposes of IR generation.
+ *
+ * In cases involving `constexpr`, a call can wind up as a constant expression. `ignoreExpr()` will
+ * not hold for such a call, since we do need to translate the call (as a constant), but we need to
+ * ignore all of the side effects of that call, since we will not actually be generating a `Call`
+ * instruction.
+ */
+private predicate ignoreSideEffects(Expr expr) {
+  ignoreExpr(expr)
+  or
+  isIRConstant(expr)
+}
+
 /**
  * Holds if `func` contains an AST that cannot be translated into IR. This is mostly used to work
  * around extractor bugs. Once the relevant extractor bugs are fixed, this predicate can be removed.
@@ -621,25 +635,16 @@ newtype TTranslatedElement =
   // The declaration/initialization part of a `ConditionDeclExpr`
   TTranslatedConditionDecl(ConditionDeclExpr expr) { not ignoreExpr(expr) } or
   // The side effects of a `Call`
-  TTranslatedCallSideEffects(Call expr) {
-    // Exclude allocations such as `malloc` (which happen to also be function calls).
-    // Both `TranslatedCallSideEffects` and `TranslatedAllocationSideEffects` generate
-    // the same side effects for its children as they both extend the `TranslatedSideEffects`
-    // class.
-    // Note: We can separate allocation side effects and call side effects into two
-    // translated elements as no call can be both a `ConstructorCall` and an `AllocationExpr`.
-    not expr instanceof AllocationExpr and
-    (
-      exists(TTranslatedArgumentExprSideEffect(expr, _, _, _)) or
-      expr instanceof ConstructorCall
-    )
+  TTranslatedCallSideEffects(CallOrAllocationExpr expr) { not ignoreSideEffects(expr) } or
+  // The non-argument-specific side effect of a `Call`
+  TTranslatedCallSideEffect(Expr expr, SideEffectOpcode opcode) {
+    not ignoreSideEffects(expr) and
+    opcode = getCallSideEffectOpcode(expr)
   } or
-  // The side effects of an allocation, i.e. `new`, `new[]` or `malloc`
-  TTranslatedAllocationSideEffects(AllocationExpr expr) { not ignoreExpr(expr) } or
   // A precise side effect of an argument to a `Call`
   TTranslatedArgumentExprSideEffect(Call call, Expr expr, int n, SideEffectOpcode opcode) {
     not ignoreExpr(expr) and
-    not ignoreExpr(call) and
+    not ignoreSideEffects(call) and
     (
       n >= 0 and expr = call.getArgument(n).getFullyConverted()
       or
@@ -650,12 +655,14 @@ newtype TTranslatedElement =
   // Constructor calls lack a qualifier (`this`) expression, so we need to handle the side effects
   // on `*this` without an `Expr`.
   TTranslatedStructorQualifierSideEffect(Call call, SideEffectOpcode opcode) {
-    not ignoreExpr(call) and
+    not ignoreSideEffects(call) and
     // Don't bother with destructor calls for now, since we won't see very many of them in the IR
     // until we start injecting implicit destructor calls.
     call instanceof ConstructorCall and
     opcode = getASideEffectOpcode(call, -1)
-  }
+  } or
+  // The side effect that initializes newly-allocated memory.
+  TTranslatedAllocationSideEffect(AllocationExpr expr) { not ignoreSideEffects(expr) }
 
 /**
  * Gets the index of the first explicitly initialized element in `initList`