C++: Refactor some side effect generation code

This change was necessary for my upcoming changes to introduce side effect instructions for indirections of smart pointers. The code to decide which parameters have which side effects appeared in both the IPA constructor for `TTranslatedSideEffect` and in `TranslatedCall`. These two versions didn't quite agree, especially once the `SideEffectFunction` model provides its own side effects instead of the defaults.
The relevant code has now been factored out into `SideEffects.qll`. This queries the model if one exists, and provides default side effects if no model exists. This fixes at least one existing issue, where we were emitting a buffer read side effect for `*this` instead of an indirect read side effect. This accounts for all of the IR diffs in the tests.

Author: Dave Bartolomeo

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

@@ -0,0 +1,109 @@
+/**
+ * Predicates to compute the modeled side effects of calls during IR construction.
+ *
+ * These are used in `TranslatedElement.qll` to generate the `TTranslatedSideEffect` instances, and
+ * also in `TranslatedCall.qll` to inject the actual side effect instructions.
+ */
+
+private import cpp
+private import semmle.code.cpp.ir.implementation.Opcode
+private import semmle.code.cpp.models.interfaces.SideEffect
+
+/**
+ * Holds if the specified call has a side effect that does not come from a `SideEffectFunction`
+ * model.
+ */
+private predicate hasDefaultSideEffect(Call call, ParameterIndex i, boolean buffer, boolean isWrite) {
+  not call.getTarget() instanceof SideEffectFunction and
+  (
+    exists(MemberFunction mfunc |
+      // A non-static member function, including a constructor or destructor, may write to `*this`,
+      // and may also read from `*this` if it is not a constructor.
+      i = -1 and
+      mfunc = call.getTarget() and
+      not mfunc.isStatic() and
+      buffer = false and
+      (
+        isWrite = false and not mfunc instanceof Constructor
+        or
+        isWrite = true and not mfunc instanceof ConstMemberFunction
+      )
+    )
+    or
+    exists(Expr expr |
+      // A pointer-like argument is assumed to read from the pointed-to buffer, and may write to the
+      // buffer as well unless the pointer points to a `const` value.
+      i >= 0 and
+      buffer = true and
+      expr = call.getArgument(i).getFullyConverted() and
+      exists(Type t | t = expr.getUnspecifiedType() |
+        t instanceof ArrayType or
+        t instanceof PointerType or
+        t instanceof ReferenceType
+      ) and
+      (
+        isWrite = true and
+        not call.getTarget().getParameter(i).getType().isDeeplyConstBelow()
+        or
+        isWrite = false
+      )
+    )
+  )
+}
+
+/**
+ * Returns a side effect opcode for parameter index `i` of the specified call.
+ *
+ * This predicate will return at most two results: one read side effect, and one write side effect.
+ */
+Opcode getASideEffectOpcode(Call call, ParameterIndex i) {
+  exists(boolean buffer |
+    (
+      call.getTarget().(SideEffectFunction).hasSpecificReadSideEffect(i, buffer)
+      or
+      not call.getTarget() instanceof SideEffectFunction and
+      hasDefaultSideEffect(call, i, buffer, false)
+    ) and
+    if exists(call.getTarget().(SideEffectFunction).getParameterSizeIndex(i))
+    then (
+      buffer = true and
+      result instanceof Opcode::SizedBufferReadSideEffect
+    ) else (
+      buffer = false and result instanceof Opcode::IndirectReadSideEffect
+      or
+      buffer = true and result instanceof Opcode::BufferReadSideEffect
+    )
+  )
+  or
+  exists(boolean buffer, boolean mustWrite |
+    (
+      call.getTarget().(SideEffectFunction).hasSpecificWriteSideEffect(i, buffer, mustWrite)
+      or
+      not call.getTarget() instanceof SideEffectFunction and
+      hasDefaultSideEffect(call, i, buffer, true) and
+      mustWrite = false
+    ) and
+    if exists(call.getTarget().(SideEffectFunction).getParameterSizeIndex(i))
+    then (
+      buffer = true and
+      mustWrite = false and
+      result instanceof Opcode::SizedBufferMayWriteSideEffect
+      or
+      buffer = true and
+      mustWrite = true and
+      result instanceof Opcode::SizedBufferMustWriteSideEffect
+    ) else (
+      buffer = false and
+      mustWrite = false and
+      result instanceof Opcode::IndirectMayWriteSideEffect
+      or
+      buffer = false and
+      mustWrite = true and
+      result instanceof Opcode::IndirectMustWriteSideEffect
+      or
+      buffer = true and mustWrite = false and result instanceof Opcode::BufferMayWriteSideEffect
+      or
+      buffer = true and mustWrite = true and result instanceof Opcode::BufferMustWriteSideEffect
+    )
+  )
+}

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

@@ -4,6 +4,7 @@ private import semmle.code.cpp.ir.implementation.internal.OperandTag
 private import semmle.code.cpp.ir.internal.CppType
 private import semmle.code.cpp.models.interfaces.SideEffect
 private import InstructionTag
+private import SideEffects
 private import TranslatedElement
 private import TranslatedExpr
 private import TranslatedFunction
@@ -424,12 +425,15 @@ class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSi
 }
 
 class TranslatedStructorCallSideEffects extends TranslatedCallSideEffects {
-  TranslatedStructorCallSideEffects() { getParent().(TranslatedStructorCall).hasQualifier() }
+  TranslatedStructorCallSideEffects() {
+    getParent().(TranslatedStructorCall).hasQualifier() and
+    getASideEffectOpcode(expr, -1) instanceof WriteSideEffectOpcode
+  }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType t) {
-    opcode instanceof Opcode::IndirectMayWriteSideEffect and
     tag instanceof OnlyInstructionTag and
-    t = getTypeForPRValue(expr.getTarget().getDeclaringType())
+    t = getTypeForPRValue(expr.getTarget().getDeclaringType()) and
+    opcode = getASideEffectOpcode(expr, -1).(WriteSideEffectOpcode)
   }
 
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
@@ -460,9 +464,11 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
   Call call;
   Expr arg;
   int index;
-  boolean write;
+  SideEffectOpcode sideEffectOpcode;
 
-  TranslatedSideEffect() { this = TTranslatedArgumentSideEffect(call, arg, index, write) }
+  TranslatedSideEffect() {
+    this = TTranslatedArgumentSideEffect(call, arg, index, sideEffectOpcode)
+  }
 
   override Locatable getAST() { result = arg }
 
@@ -472,13 +478,13 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
 
   int getArgumentIndex() { result = index }
 
-  predicate isWrite() { write = true }
+  predicate isWrite() { sideEffectOpcode instanceof WriteSideEffectOpcode }
 
   override string toString() {
-    write = true and
+    isWrite() and
     result = "(write side effect for " + arg.toString() + ")"
     or
-    write = false and
+    not isWrite() and
     result = "(read side effect for " + arg.toString() + ")"
   }
 
@@ -489,29 +495,31 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
   override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
-    isWrite() and
-    hasSpecificWriteSideEffect(opcode) and
-    tag = OnlyInstructionTag() and
     (
-      opcode instanceof BufferAccessOpcode and
-      type = getUnknownType()
-      or
-      not opcode instanceof BufferAccessOpcode and
-      exists(Type baseType | baseType = arg.getUnspecifiedType().(DerivedType).getBaseType() |
-        if baseType instanceof VoidType
-        then type = getUnknownType()
-        else type = getTypeForPRValueOrUnknown(baseType)
+      tag = OnlyInstructionTag() and
+      opcode = sideEffectOpcode
+    ) and
+    (
+      isWrite() and
+      (
+        opcode instanceof BufferAccessOpcode and
+        type = getUnknownType()
+        or
+        not opcode instanceof BufferAccessOpcode and
+        exists(Type baseType | baseType = arg.getUnspecifiedType().(DerivedType).getBaseType() |
+          if baseType instanceof VoidType
+          then type = getUnknownType()
+          else type = getTypeForPRValueOrUnknown(baseType)
+        )
+        or
+        index = -1 and
+        not arg.getUnspecifiedType() instanceof DerivedType and
+        type = getTypeForPRValueOrUnknown(arg.getUnspecifiedType())
       )
       or
-      index = -1 and
-      not arg.getUnspecifiedType() instanceof DerivedType and
-      type = getTypeForPRValueOrUnknown(arg.getUnspecifiedType())
+      not isWrite() and
+      type = getVoidType()
     )
-    or
-    not isWrite() and
-    hasSpecificReadSideEffect(opcode) and
-    tag = OnlyInstructionTag() and
-    type = getVoidType()
   }
 
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
@@ -535,7 +543,7 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
 
   override CppType getInstructionMemoryOperandType(InstructionTag tag, TypedOperandTag operandTag) {
     not isWrite() and
-    if hasSpecificReadSideEffect(any(BufferAccessOpcode op))
+    if sideEffectOpcode instanceof BufferAccessOpcode
     then
       result = getUnknownType() and
       tag instanceof OnlyInstructionTag and
@@ -557,56 +565,6 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
       )
   }
 
-  predicate hasSpecificWriteSideEffect(Opcode op) {
-    exists(boolean buffer, boolean mustWrite |
-      if exists(call.getTarget().(SideEffectFunction).getParameterSizeIndex(index))
-      then
-        call.getTarget().(SideEffectFunction).hasSpecificWriteSideEffect(index, true, mustWrite) and
-        buffer = true and
-        (
-          mustWrite = false and op instanceof Opcode::SizedBufferMayWriteSideEffect
-          or
-          mustWrite = true and op instanceof Opcode::SizedBufferMustWriteSideEffect
-        )
-      else (
-        call.getTarget().(SideEffectFunction).hasSpecificWriteSideEffect(index, buffer, mustWrite) and
-        (
-          buffer = true and mustWrite = false and op instanceof Opcode::BufferMayWriteSideEffect
-          or
-          buffer = false and mustWrite = false and op instanceof Opcode::IndirectMayWriteSideEffect
-          or
-          buffer = true and mustWrite = true and op instanceof Opcode::BufferMustWriteSideEffect
-          or
-          buffer = false and mustWrite = true and op instanceof Opcode::IndirectMustWriteSideEffect
-        )
-      )
-    )
-    or
-    not call.getTarget() instanceof SideEffectFunction and
-    getArgumentIndex() != -1 and
-    op instanceof Opcode::BufferMayWriteSideEffect
-    or
-    not call.getTarget() instanceof SideEffectFunction and
-    getArgumentIndex() = -1 and
-    op instanceof Opcode::IndirectMayWriteSideEffect
-  }
-
-  predicate hasSpecificReadSideEffect(Opcode op) {
-    exists(boolean buffer |
-      call.getTarget().(SideEffectFunction).hasSpecificReadSideEffect(index, buffer) and
-      if exists(call.getTarget().(SideEffectFunction).getParameterSizeIndex(index))
-      then buffer = true and op instanceof Opcode::SizedBufferReadSideEffect
-      else (
-        buffer = true and op instanceof Opcode::BufferReadSideEffect
-        or
-        buffer = false and op instanceof Opcode::IndirectReadSideEffect
-      )
-    )
-    or
-    not call.getTarget() instanceof SideEffectFunction and
-    op instanceof Opcode::BufferReadSideEffect
-  }
-
   override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
     tag = OnlyInstructionTag() and
     result = getTranslatedCallInstruction(call)

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

@@ -12,6 +12,7 @@ private import TranslatedStmt
 private import TranslatedExpr
 private import IRConstruction
 private import semmle.code.cpp.models.interfaces.SideEffect
+private import SideEffects
 
 /**
  * Gets the "real" parent of `expr`. This predicate treats conversions as if
@@ -635,46 +636,15 @@ newtype TTranslatedElement =
   // 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, boolean isWrite) {
-    (
-      expr = call.getArgument(n).getFullyConverted()
-      or
-      expr = call.getQualifier().getFullyConverted() and
-      n = -1 and
-      // Exclude calls to static member functions. They don't modify the qualifier
-      not exists(MemberFunction func | func = call.getTarget() and func.isStatic())
-    ) and
+  TTranslatedArgumentSideEffect(Call call, Expr expr, int n, SideEffectOpcode opcode) {
+    not ignoreExpr(expr) and
+    not ignoreExpr(call) and
     (
-      call.getTarget().(SideEffectFunction).hasSpecificReadSideEffect(n, _) and
-      isWrite = false
-      or
-      call.getTarget().(SideEffectFunction).hasSpecificWriteSideEffect(n, _, _) and
-      isWrite = true
-      or
-      not call.getTarget() instanceof SideEffectFunction and
-      exists(Type t | t = expr.getUnspecifiedType() |
-        t instanceof ArrayType or
-        t instanceof PointerType or
-        t instanceof ReferenceType
-      ) and
-      (
-        isWrite = true and
-        not call.getTarget().getParameter(n).getType().isDeeplyConstBelow()
-        or
-        isWrite = false
-      )
+      n >= 0 and expr = call.getArgument(n).getFullyConverted()
       or
-      not call.getTarget() instanceof SideEffectFunction and
-      n = -1 and
-      (
-        isWrite = true and
-        not call.getTarget() instanceof ConstMemberFunction
-        or
-        isWrite = false
-      )
+      n = -1 and expr = call.getQualifier().getFullyConverted()
     ) and
-    not ignoreExpr(expr) and
-    not ignoreExpr(call)
+    opcode = getASideEffectOpcode(call, n)
   }
 
 /**