C++: Fix MemoryLocation with multiple VirtualVariables

While investigating a bug with `TInstruction` sharing, I discovered that we had a case where alias analysis could create two `VirtualVariable`s for the same `Allocation`. For an indirect parameter allocation, we were using the type of the pointer variable as the type of the indirect allocation, instead of just `Unknown`. If the `IRType` of the pointer variable was the same type as the type of at least one access to the indirect allocation, we'd create both an `EntireAllocationVirtualVariable` and a `VariableVirtualVariable` for the allocation.

I added a new consistency test to guard against this in the future. This also turned out to be the root cause of the one existing known consistency failure in the IR tests.

Author: Dave Bartolomeo

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll

@@ -90,7 +90,7 @@ class IndirectParameterAllocation extends Allocation, TIndirectParameterAllocati
 
   final override string getUniqueId() { result = var.getUniqueId() }
 
-  final override IRType getIRType() { result = var.getIRType() }
+  final override IRType getIRType() { result instanceof IRUnknownType }
 
   final override predicate isReadOnly() { none() }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll

@@ -913,6 +913,9 @@ private module CachedForDebugging {
 }
 
 module SSAConsistency {
+  /**
+   * Holds if a `MemoryOperand` has more than one `MemoryLocation` assigned by alias analysis.
+   */
   query predicate multipleOperandMemoryLocations(
     OldIR::MemoryOperand operand, string message, OldIR::IRFunction func, string funcText
   ) {
@@ -925,6 +928,9 @@ module SSAConsistency {
     )
   }
 
+  /**
+   * Holds if a `MemoryLocation` does not have an associated `VirtualVariable`.
+   */
   query predicate missingVirtualVariableForMemoryLocation(
     Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText
   ) {
@@ -933,4 +939,25 @@ module SSAConsistency {
     funcText = Language::getIdentityString(func.getFunction()) and
     message = "Memory location has no virtual variable in function '$@'."
   }
+
+  /**
+   * Holds if a `MemoryLocation` is a member of more than one `VirtualVariable`.
+   */
+  query predicate multipleVirtualVariablesForMemoryLocation(
+    Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText
+  ) {
+    exists(int vvarCount |
+      vvarCount = strictcount(location.getVirtualVariable()) and
+      vvarCount > 1 and
+      func = location.getIRFunction() and
+      funcText = Language::getIdentityString(func.getFunction()) and
+      message =
+        "Memory location has " + vvarCount.toString() + " virtual variables in function '$@': (" +
+          concat(Alias::VirtualVariable vvar |
+            vvar = location.getVirtualVariable()
+          |
+            vvar.toString(), ", "
+          ) + ")."
+    )
+  }
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll

@@ -913,6 +913,9 @@ private module CachedForDebugging {
 }
 
 module SSAConsistency {
+  /**
+   * Holds if a `MemoryOperand` has more than one `MemoryLocation` assigned by alias analysis.
+   */
   query predicate multipleOperandMemoryLocations(
     OldIR::MemoryOperand operand, string message, OldIR::IRFunction func, string funcText
   ) {
@@ -925,6 +928,9 @@ module SSAConsistency {
     )
   }
 
+  /**
+   * Holds if a `MemoryLocation` does not have an associated `VirtualVariable`.
+   */
   query predicate missingVirtualVariableForMemoryLocation(
     Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText
   ) {
@@ -933,4 +939,25 @@ module SSAConsistency {
     funcText = Language::getIdentityString(func.getFunction()) and
     message = "Memory location has no virtual variable in function '$@'."
   }
+
+  /**
+   * Holds if a `MemoryLocation` is a member of more than one `VirtualVariable`.
+   */
+  query predicate multipleVirtualVariablesForMemoryLocation(
+    Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText
+  ) {
+    exists(int vvarCount |
+      vvarCount = strictcount(location.getVirtualVariable()) and
+      vvarCount > 1 and
+      func = location.getIRFunction() and
+      funcText = Language::getIdentityString(func.getFunction()) and
+      message =
+        "Memory location has " + vvarCount.toString() + " virtual variables in function '$@': (" +
+          concat(Alias::VirtualVariable vvar |
+            vvar = location.getVirtualVariable()
+          |
+            vvar.toString(), ", "
+          ) + ")."
+    )
+  }
 }

cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency.expected

@@ -1,2 +1,3 @@
 multipleOperandMemoryLocations
 missingVirtualVariableForMemoryLocation
+multipleVirtualVariablesForMemoryLocation

cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency_unsound.expected

@@ -1,2 +1,3 @@
 multipleOperandMemoryLocations
 missingVirtualVariableForMemoryLocation
+multipleVirtualVariablesForMemoryLocation

cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected

@@ -1,2 +1,3 @@
 multipleOperandMemoryLocations
 missingVirtualVariableForMemoryLocation
+multipleVirtualVariablesForMemoryLocation

cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency_unsound.expected

@@ -1,2 +1,3 @@
 multipleOperandMemoryLocations
 missingVirtualVariableForMemoryLocation
+multipleVirtualVariablesForMemoryLocation

cpp/ql/test/library-tests/ir/ssa/aliased_ssa_consistency_unsound.expected

@@ -20,7 +20,6 @@ switchInstructionWithoutDefaultEdge
 notMarkedAsConflated
 wronglyMarkedAsConflated
 invalidOverlap
-| ssa.cpp:301:27:301:30 | SideEffect | MemoryOperand 'SideEffect' has a `getDefinitionOverlap()` of 'MayPartiallyOverlap'. | ssa.cpp:301:5:301:8 | IR: main | int main(int, char**) |
 missingCanonicalLanguageType
 multipleCanonicalLanguageTypes
 missingIRType

cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected

@@ -1426,7 +1426,7 @@ ssa.cpp:
 #  302|     m302_8(unknown)        = Chi                              : total:m301_4, partial:m302_7
 #  302|     v302_9(void)           = ^BufferReadSideEffect[1]         : &:r302_5, ~m301_10
 #  302|     m302_10(unknown)       = ^BufferMayWriteSideEffect[1]     : &:r302_5
-#  302|     m302_11(char *)        = Chi                              : total:m301_10, partial:m302_10
+#  302|     m302_11(unknown)       = Chi                              : total:m301_10, partial:m302_10
 #  303|     r303_1(glval<unknown>) = FunctionAddress[unknownFunction] : 
 #  303|     r303_2(glval<int>)     = VariableAddress[argc]            : 
 #  303|     r303_3(int)            = Load                             : &:r303_2, m301_6
@@ -1437,7 +1437,7 @@ ssa.cpp:
 #  303|     m303_8(unknown)        = Chi                              : total:m302_8, partial:m303_7
 #  303|     v303_9(void)           = ^BufferReadSideEffect[1]         : &:r303_5, ~m302_11
 #  303|     m303_10(unknown)       = ^BufferMayWriteSideEffect[1]     : &:r303_5
-#  303|     m303_11(char *)        = Chi                              : total:m302_11, partial:m303_10
+#  303|     m303_11(unknown)       = Chi                              : total:m302_11, partial:m303_10
 #  304|     r304_1(glval<int>)     = VariableAddress[#return]         : 
 #  304|     r304_2(glval<char **>) = VariableAddress[argv]            : 
 #  304|     r304_3(char **)        = Load                             : &:r304_2, m301_8

cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency.expected

@@ -1,2 +1,3 @@
 multipleOperandMemoryLocations
 missingVirtualVariableForMemoryLocation
+multipleVirtualVariablesForMemoryLocation