Make side effects for constructor calls use same mechanism as other arguments

This commit is yet another step to fixing the order of IR side effect instructions. Instead of having a special `StructorCallSideEffects` class for the call itself, I've introduced a `TranslatedStructorCallQualifierSideEffect` class that shares a bunch of common code with `TranslatedArgumentExprSideEffect`, but handles the case where there's no `Expr` for the qualifier of the constructor call. Because this class uses the same ordering as regular argument side effects, these side effects now appear in the correct order, reads before writes.

The test expectations have changed to reflect the new, correct order.

Author: Dave Bartolomeo

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

@@ -406,42 +406,6 @@ class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSi
   }
 }
 
-class TranslatedStructorCallSideEffects extends TranslatedCallSideEffects {
-  TranslatedStructorCallSideEffects() {
-    getParent().(TranslatedStructorCall).hasQualifier() and
-    getASideEffectOpcode(expr, -1) instanceof WriteSideEffectOpcode
-  }
-
-  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType t) {
-    tag instanceof OnlyInstructionTag and
-    t = getTypeForPRValue(expr.getTarget().getDeclaringType()) and
-    opcode = getASideEffectOpcode(expr, -1).(WriteSideEffectOpcode)
-  }
-
-  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
-    (
-      if exists(getChild(0))
-      then result = getChild(0).getFirstInstruction()
-      else result = getParent().getChildSuccessor(this)
-    ) and
-    tag = OnlyInstructionTag() and
-    kind instanceof GotoEdge
-  }
-
-  override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
-
-  override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
-    tag instanceof OnlyInstructionTag and
-    operandTag instanceof AddressOperandTag and
-    result = getParent().(TranslatedStructorCall).getQualifierResult()
-  }
-
-  final override int getInstructionIndex(InstructionTag tag) {
-    tag = OnlyInstructionTag() and
-    result = -1
-  }
-}
-
 /** Returns the sort group index for argument read side effects. */
 private int argumentReadGroup() { result = 1 }
 
@@ -502,19 +466,22 @@ abstract class TranslatedSideEffect extends TranslatedElement {
 /**
  * The IR translation of a single argument side effect for a call.
  */
-class TranslatedArgumentSideEffect extends TranslatedSideEffect, TTranslatedArgumentSideEffect {
+abstract class TranslatedArgumentSideEffect extends TranslatedSideEffect {
   Call call;
-  Expr arg;
   int index;
   SideEffectOpcode sideEffectOpcode;
 
-  TranslatedArgumentSideEffect() {
-    this = TTranslatedArgumentSideEffect(call, arg, index, sideEffectOpcode)
-  }
-
-  override Locatable getAST() { result = arg }
+  // All subclass charpreds must bind the `index` field.
+  bindingset[index]
+  TranslatedArgumentSideEffect() { any() }
 
-  Expr getExpr() { result = arg }
+  override string toString() {
+    isWrite() and
+    result = "(write side effect for " + getArgString() + ")"
+    or
+    not isWrite() and
+    result = "(read side effect for " + getArgString() + ")"
+  }
 
   override Call getPrimaryExpr() { result = call }
 
@@ -523,17 +490,29 @@ class TranslatedArgumentSideEffect extends TranslatedSideEffect, TTranslatedArgu
     if isWrite() then group = argumentWriteGroup() else group = argumentReadGroup()
   }
 
-  predicate isWrite() { sideEffectOpcode instanceof WriteSideEffectOpcode }
+  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = getTranslatedCallInstruction(call)
+  }
 
-  override string toString() {
-    isWrite() and
-    result = "(write side effect for " + arg.toString() + ")"
-    or
-    not isWrite() and
-    result = "(read side effect for " + arg.toString() + ")"
+  final override int getInstructionIndex(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = index
   }
 
-  override predicate sideEffectInstruction(Opcode opcode, CppType type) {
+  /**
+   * Gets the `TranslatedFunction` containing this expression.
+   */
+  final TranslatedFunction getEnclosingFunction() {
+    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
     (
       isWrite() and
@@ -542,36 +521,21 @@ class TranslatedArgumentSideEffect extends TranslatedSideEffect, TTranslatedArgu
         type = getUnknownType()
         or
         not opcode instanceof BufferAccessOpcode and
-        exists(Type baseType | baseType = arg.getUnspecifiedType().(DerivedType).getBaseType() |
-          if baseType instanceof VoidType
+        exists(Type indirectionType | indirectionType = getIndirectionType() |
+          if indirectionType instanceof VoidType
           then type = getUnknownType()
-          else type = getTypeForPRValueOrUnknown(baseType)
+          else type = getTypeForPRValueOrUnknown(indirectionType)
         )
-        or
-        index = -1 and
-        not arg.getUnspecifiedType() instanceof DerivedType and
-        type = getTypeForPRValueOrUnknown(arg.getUnspecifiedType())
       )
       or
       not isWrite() and
       type = getVoidType()
     )
   }
 
-  override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
-    tag instanceof OnlyInstructionTag and
-    operandTag instanceof AddressOperandTag and
-    result = getTranslatedExpr(arg).getResult()
-    or
-    tag instanceof OnlyInstructionTag and
-    operandTag instanceof BufferSizeOperandTag and
-    result =
-      getTranslatedExpr(call.getArgument(call.getTarget()
-              .(SideEffectFunction)
-              .getParameterSizeIndex(index)).getFullyConverted()).getResult()
-  }
-
-  override CppType getInstructionMemoryOperandType(InstructionTag tag, TypedOperandTag operandTag) {
+  final override CppType getInstructionMemoryOperandType(
+    InstructionTag tag, TypedOperandTag operandTag
+  ) {
     not isWrite() and
     if sideEffectOpcode instanceof BufferAccessOpcode
     then
@@ -581,12 +545,7 @@ class TranslatedArgumentSideEffect extends TranslatedSideEffect, TTranslatedArgu
     else
       exists(Type operandType |
         tag instanceof OnlyInstructionTag and
-        operandType = arg.getType().getUnspecifiedType().(DerivedType).getBaseType() and
-        operandTag instanceof SideEffectOperandTag
-        or
-        tag instanceof OnlyInstructionTag and
-        operandType = arg.getType().getUnspecifiedType() and
-        not operandType instanceof DerivedType and
+        operandType = getIndirectionType() and
         operandTag instanceof SideEffectOperandTag
       |
         // If the type we select is an incomplete type (e.g. a forward-declared `struct`), there will
@@ -595,25 +554,87 @@ class TranslatedArgumentSideEffect extends TranslatedSideEffect, TTranslatedArgu
       )
   }
 
-  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
-    tag = OnlyInstructionTag() and
-    result = getTranslatedCallInstruction(call)
+  final override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
+    tag instanceof OnlyInstructionTag and
+    operandTag instanceof AddressOperandTag and
+    result = getArgInstruction()
+    or
+    tag instanceof OnlyInstructionTag and
+    operandTag instanceof BufferSizeOperandTag and
+    result =
+      getTranslatedExpr(call.getArgument(call.getTarget()
+              .(SideEffectFunction)
+              .getParameterSizeIndex(index)).getFullyConverted()).getResult()
   }
 
-  final override int getInstructionIndex(InstructionTag tag) {
-    tag = OnlyInstructionTag() and
-    result = index
+  /** Holds if this side effect is a write side effect, rather than a read side effect. */
+  final predicate isWrite() { sideEffectOpcode instanceof WriteSideEffectOpcode }
+
+  /** Gets a text representation of the argument. */
+  abstract string getArgString();
+
+  /** Gets the `Instruction` whose result is the value of the argument. */
+  abstract Instruction getArgInstruction();
+
+  /** Gets the type pointed to by the argument. */
+  abstract Type getIndirectionType();
+}
+
+/**
+ * The IR translation of an argument side effect where the argument has an `Expr` object in the AST.
+ *
+ * This generally applies to all positional arguments, as well as qualifier (`this`) arguments for
+ * calls other than constructor calls.
+ */
+class TranslatedArgumentExprSideEffect extends TranslatedArgumentSideEffect,
+  TTranslatedArgumentExprSideEffect {
+  Expr arg;
+
+  TranslatedArgumentExprSideEffect() {
+    this = TTranslatedArgumentExprSideEffect(call, arg, index, sideEffectOpcode)
   }
 
-  /**
-   * Gets the `TranslatedFunction` containing this expression.
-   */
-  final TranslatedFunction getEnclosingFunction() {
-    result = getTranslatedFunction(arg.getEnclosingFunction())
+  final override Locatable getAST() { result = arg }
+
+  final override Type getIndirectionType() {
+    result = arg.getUnspecifiedType().(DerivedType).getBaseType()
+    or
+    // Sometimes the qualifier type gets the type of the class itself, rather than a pointer to the
+    // class.
+    index = -1 and
+    not arg.getUnspecifiedType() instanceof DerivedType and
+    result = arg.getUnspecifiedType()
   }
 
-  /**
-   * Gets the `Function` containing this expression.
-   */
-  override Function getFunction() { result = arg.getEnclosingFunction() }
+  final override string getArgString() { result = arg.toString() }
+
+  final override Instruction getArgInstruction() { result = getTranslatedExpr(arg).getResult() }
+}
+
+/**
+ * The IR translation of an argument side effect for `*this` on a call, where there is no `Expr`
+ * object that represents the `this` argument.
+ *
+ * The applies only to constructor calls, as the AST has explioit qualifier `Expr`s for all other
+ * calls to non-static member functions.
+ */
+class TranslatedStructorQualifierSideEffect extends TranslatedArgumentSideEffect,
+  TTranslatedStructorQualifierSideEffect {
+  TranslatedStructorQualifierSideEffect() {
+    this = TTranslatedStructorQualifierSideEffect(call, sideEffectOpcode) and
+    index = -1
+  }
+
+  final override Locatable getAST() { result = call }
+
+  final override Type getIndirectionType() { result = call.getTarget().getDeclaringType() }
+
+  final override string getArgString() { result = "this" }
+
+  final override Instruction getArgInstruction() {
+    exists(TranslatedStructorCall structorCall |
+      structorCall.getExpr() = call and
+      result = structorCall.getQualifierResult()
+    )
+  }
 }

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

@@ -630,14 +630,14 @@ newtype TTranslatedElement =
     // translated elements as no call can be both a `ConstructorCall` and an `AllocationExpr`.
     not expr instanceof AllocationExpr and
     (
-      exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
+      exists(TTranslatedArgumentExprSideEffect(expr, _, _, _)) or
       expr instanceof ConstructorCall
     )
   } 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`
-  TTranslatedArgumentSideEffect(Call call, Expr expr, int n, SideEffectOpcode opcode) {
+  TTranslatedArgumentExprSideEffect(Call call, Expr expr, int n, SideEffectOpcode opcode) {
     not ignoreExpr(expr) and
     not ignoreExpr(call) and
     (
@@ -646,6 +646,15 @@ newtype TTranslatedElement =
       n = -1 and expr = call.getQualifier().getFullyConverted()
     ) and
     opcode = getASideEffectOpcode(call, n)
+  } or
+  // 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
+    // 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)
   }
 
 /**