From ba72a1cde7a8a57f18da2ac3ecccbf44fd630ae2 Mon Sep 17 00:00:00 2001
From: Dave Bartolomeo <dbartol@github.com>
Date: Fri, 3 Sep 2021 11:31:14 -0400
Subject: [PATCH] Make `TranslatedSideEffect` abstract

This is step two of fixing the ordering of call side effects. This commit refactors the existing `TranslatedSideEffect` class into an abstract `TranslatedSideEffect` class, which contains functionality common to all kinds of side effect, and a concrete `TranslatedArgumentSideEffect` class, which is the implementation of argument side effects. A future commit will add additional concrete classes for conservative call side effects and allocation side effects.

This change has zero diffs to the generated IR.
---
 .../raw/internal/TranslatedCall.qll           | 97 ++++++++++++++-----
 1 file changed, 72 insertions(+), 25 deletions(-)

diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll
index 3c130eed056..c63db682cdf 100644
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll
@@ -219,14 +219,11 @@ abstract class TranslatedSideEffects extends TranslatedElement {
 
   override TranslatedElement getChild(int i) {
     result =
-      rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
-        (
-          tse.getCall() = getExpr() and
-          tse.getArgumentIndex() = index and
-          if tse.isWrite() then isWrite = 1 else isWrite = 0
-        )
+      rank[i + 1](TranslatedSideEffect tse, int group, int indexInGroup |
+        tse.getPrimaryExpr() = getExpr() and
+        tse.sortOrder(group, indexInGroup)
       |
-        tse order by isWrite, index
+        tse order by group, indexInGroup
       )
   }
 
@@ -445,13 +442,73 @@ class TranslatedStructorCallSideEffects extends TranslatedCallSideEffects {
   }
 }
 
-class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEffect {
+/** Returns the sort group index for argument read side effects. */
+private int argumentReadGroup() { result = 1 }
+
+/** Returns the sort group index for conservative call side effects. */
+private int callSideEffectGroup() {
+  result = 0 // Make this group first for now to preserve the existing ordering
+}
+
+/** Returns the sort group index for argument write side effects. */
+private int argumentWriteGroup() { result = 2 }
+
+/** Returns the sort group index for dynamic allocation side effects. */
+private int initializeAllocationGroup() { result = 3 }
+
+/**
+ * The IR translation of a single side effect of a call.
+ */
+abstract class TranslatedSideEffect extends TranslatedElement {
+  final override TranslatedElement getChild(int n) { none() }
+
+  final override Instruction getChildSuccessor(TranslatedElement child) { none() }
+
+  final override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
+
+  final override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
+    tag = OnlyInstructionTag() and
+    sideEffectInstruction(opcode, type)
+  }
+
+  final override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
+    result = getParent().getChildSuccessor(this) and
+    tag = OnlyInstructionTag() and
+    kind instanceof GotoEdge
+  }
+
+  /**
+   * Gets the expression that caused this side effect.
+   *
+   * All side effects with the same `getPrimaryExpr()` will appear in the same contiguous sequence
+   * in the IR.
+   */
+  abstract Expr getPrimaryExpr();
+
+  /**
+   * Gets the order in which this side effect should be sorted with respect to other side effects
+   * for the same expression.
+   *
+   * Side effects are sorted first by `group`, and then by `indexInGroup`.
+   */
+  abstract predicate sortOrder(int group, int indexInGroup);
+
+  /**
+   * Gets the opcode and result type for the side effect instruction.
+   */
+  abstract predicate sideEffectInstruction(Opcode opcode, CppType type);
+}
+
+/**
+ * The IR translation of a single argument side effect for a call.
+ */
+class TranslatedArgumentSideEffect extends TranslatedSideEffect, TTranslatedArgumentSideEffect {
   Call call;
   Expr arg;
   int index;
   SideEffectOpcode sideEffectOpcode;
 
-  TranslatedSideEffect() {
+  TranslatedArgumentSideEffect() {
     this = TTranslatedArgumentSideEffect(call, arg, index, sideEffectOpcode)
   }
 
@@ -459,9 +516,12 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
 
   Expr getExpr() { result = arg }
 
-  Call getCall() { result = call }
+  override Call getPrimaryExpr() { result = call }
 
-  int getArgumentIndex() { result = index }
+  override predicate sortOrder(int group, int indexInGroup) {
+    indexInGroup = index and
+    if isWrite() then group = argumentWriteGroup() else group = argumentReadGroup()
+  }
 
   predicate isWrite() { sideEffectOpcode instanceof WriteSideEffectOpcode }
 
@@ -473,14 +533,7 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
     result = "(read side effect for " + arg.toString() + ")"
   }
 
-  override TranslatedElement getChild(int n) { none() }
-
-  override Instruction getChildSuccessor(TranslatedElement child) { none() }
-
-  override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
-
-  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
-    tag = OnlyInstructionTag() and
+  override predicate sideEffectInstruction(Opcode opcode, CppType type) {
     opcode = sideEffectOpcode and
     (
       isWrite() and
@@ -505,12 +558,6 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
     )
   }
 
-  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
-    result = getParent().getChildSuccessor(this) and
-    tag = OnlyInstructionTag() and
-    kind instanceof GotoEdge
-  }
-
   override Instruction getInstructionRegisterOperand(InstructionTag tag, OperandTag operandTag) {
     tag instanceof OnlyInstructionTag and
     operandTag instanceof AddressOperandTag and