C++: Fix PR feedback

Author: dave-bartolomeo

cpp/ql/src/semmle/code/cpp/ir/implementation/SSA.md

@@ -121,15 +121,15 @@ other `MemoryLocation`.
 #### Aliased SSA
 The current memory model used to construct Aliased SSA models every memory access. There are two
 kinds of `MemoryLocation`:
-- `VariableMemoryAccess` represents an access to a known `IRVariable` with a specific type, at a bit
-offset that may or may not be a known constant. `VariableMemoryAccess` represents any access to a
+- `VariableMemoryLocation` represents an access to a known `IRVariable` with a specific type, at a bit
+offset that may or may not be a known constant. `VariableMemoryLocation` represents any access to a
 known `IRVariable` even if that variable's address escapes.
 - `UnknownMemoryLocation` represents an access where the memory being accessed is not known to be part
 of a single specific `IRVariable`.
 
 In addition, there are two different kinds of `VirtualVariable`:
 - `VariableVirtualVariable` represents an `IRVariable` whose address does not escape. The
-`VariableVirtualVariable` is just the `VariableMemoryAccess` that represents an access to the entire
+`VariableVirtualVariable` is just the `VariableMemoryLocation` that represents an access to the entire
 `IRVariable` with its declared type.
 - `UnknownVirtualVariable` represents all memory that is not covered by a `VariableVirtualVariable`.
 This includes the `UnknownMemoryLocation`, as well as any `VariableMemoryLocation` whose

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

@@ -38,6 +38,13 @@ private newtype TMemoryLocation =
   TUnknownMemoryLocation(FunctionIR funcIR) or
   TUnknownVirtualVariable(FunctionIR funcIR)
 
+/**
+ * Represents the memory location accessed by a memory operand or memory result. In this implementation, the location is
+ * one of the following:
+ * - `VariableMemoryLocation` - A location within a known `IRVariable`, at an offset that is either a constant or is
+ * unknown.
+ * - `UnknownMemoryLocation` - A location not known to be within a specific `IRVariable`.
+ */
 abstract class MemoryLocation extends TMemoryLocation {
   abstract string toString();
 
@@ -53,7 +60,7 @@ abstract class VirtualVariable extends MemoryLocation {
 
 /**
  * An access to memory within a single known `IRVariable`. The variable may be either an unescaped variable
- * (with its own `VirtualIRVariable`) or an escaped variable (assiged to `UnknownVirtualVariable`).
+ * (with its own `VirtualIRVariable`) or an escaped variable (assigned to `UnknownVirtualVariable`).
  */
 class VariableMemoryLocation extends TVariableMemoryLocation, MemoryLocation {
   IRVariable var;
@@ -102,10 +109,15 @@ class VariableMemoryLocation extends TVariableMemoryLocation, MemoryLocation {
    */
   final predicate coversEntireVariable() {
     startBitOffset = 0 and
-    Ints::isEQ(endBitOffset, Ints::mul(var.getType().getSize(), 8))
+    endBitOffset = var.getType().getSize() * 8
   }
 }
 
+/**
+ * Represents the `MemoryLocation` for an `IRVariable` that acts as its own `VirtualVariable`. Includes any
+ * `VariableMemoryLocation` that exactly overlaps its entire `IRVariable`, and only if that `IRVariable` does not
+ * escape.
+ */
 class VariableVirtualVariable extends VariableMemoryLocation, VirtualVariable {
   VariableVirtualVariable() {
     not variableAddressEscapes(var) and

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

@@ -388,10 +388,17 @@ private import PhiInsertion
  * location and there is a use of that location reachable from that block without an intervening definition of the
  * location.
  * Within the approach outlined above, we treat a location slightly differently depending on whether or not it is a
- * virtual variable. For a virtual variable, 
- * location into the beginning of a block
+ * virtual variable. For a virtual variable, we will insert a `Phi` instruction on the dominance frontier if there is
+ * a use of any member location of that virtual variable that is reachable from the `Phi` instruction. For a location
+ * that is not a virtual variable, we insert a `Phi` instruction only if there is an exactly-overlapping use of the
+ * location reachable from the `Phi` instruction. This ensures that we insert a `Phi` instruction for a non-virtual
+ * variable only if doing so would allow dataflow analysis to get a more precise result than if we just used a `Phi`
+ * instruction for the virtual variable as a whole.
  */
 private module PhiInsertion {
+  /**
+   * Holds if a `Phi` instruction needs to be inserted for location `defLocation` at the beginning of block `phiBlock`.
+   */
   predicate definitionHasPhiNode(Alias::MemoryLocation defLocation, OldBlock phiBlock) {
     exists(OldBlock defBlock |
       phiBlock = Dominance::getDominanceFrontier(defBlock) and
@@ -402,8 +409,8 @@ private module PhiInsertion {
   }
 
   /**
-    * Holds if the virtual variable `vvar` has a definition in block `block`, either because of an existing instruction
-    * or because of a Phi node.
+    * Holds if the memory location `defLocation` has a definition in block `block`, either because of an existing
+    * instruction, a `Phi` node, or a `Chi` node. 
     */
   private predicate definitionHasDefinitionInBlock(Alias::MemoryLocation defLocation, OldBlock block) {
     definitionHasPhiNode(defLocation, block) or
@@ -501,10 +508,10 @@ private module PhiInsertion {
 private import DefUse
 
 /**
- * Module containing the predicates that connect uses to their reaching definition. The reaching definitios are computed
- * separately for each unique use `MemoryLocation`. An instruction is treated as a definition of a use location if the
- * defined location overlaps the use location in any way. Thus, a single instruction may serve as a definition for
- * multiple use locations, since a single definition location may overlap many use locations.
+ * Module containing the predicates that connect uses to their reaching definition. The reaching definitions are
+ * computed separately for each unique use `MemoryLocation`. An instruction is treated as a definition of a use location
+ * if the defined location overlaps the use location in any way. Thus, a single instruction may serve as a definition
+ * for multiple use locations, since a single definition location may overlap many use locations.
  * 
  * Definitions and uses are identified by a block and an integer "offset". An offset of -1 indicates the definition
  * from a `Phi` instruction at the beginning of the block. An offset of 2*i indicates a definition or use on the

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

@@ -388,10 +388,17 @@ private import PhiInsertion
  * location and there is a use of that location reachable from that block without an intervening definition of the
  * location.
  * Within the approach outlined above, we treat a location slightly differently depending on whether or not it is a
- * virtual variable. For a virtual variable, 
- * location into the beginning of a block
+ * virtual variable. For a virtual variable, we will insert a `Phi` instruction on the dominance frontier if there is
+ * a use of any member location of that virtual variable that is reachable from the `Phi` instruction. For a location
+ * that is not a virtual variable, we insert a `Phi` instruction only if there is an exactly-overlapping use of the
+ * location reachable from the `Phi` instruction. This ensures that we insert a `Phi` instruction for a non-virtual
+ * variable only if doing so would allow dataflow analysis to get a more precise result than if we just used a `Phi`
+ * instruction for the virtual variable as a whole.
  */
 private module PhiInsertion {
+  /**
+   * Holds if a `Phi` instruction needs to be inserted for location `defLocation` at the beginning of block `phiBlock`.
+   */
   predicate definitionHasPhiNode(Alias::MemoryLocation defLocation, OldBlock phiBlock) {
     exists(OldBlock defBlock |
       phiBlock = Dominance::getDominanceFrontier(defBlock) and
@@ -402,8 +409,8 @@ private module PhiInsertion {
   }
 
   /**
-    * Holds if the virtual variable `vvar` has a definition in block `block`, either because of an existing instruction
-    * or because of a Phi node.
+    * Holds if the memory location `defLocation` has a definition in block `block`, either because of an existing
+    * instruction, a `Phi` node, or a `Chi` node. 
     */
   private predicate definitionHasDefinitionInBlock(Alias::MemoryLocation defLocation, OldBlock block) {
     definitionHasPhiNode(defLocation, block) or
@@ -501,10 +508,10 @@ private module PhiInsertion {
 private import DefUse
 
 /**
- * Module containing the predicates that connect uses to their reaching definition. The reaching definitios are computed
- * separately for each unique use `MemoryLocation`. An instruction is treated as a definition of a use location if the
- * defined location overlaps the use location in any way. Thus, a single instruction may serve as a definition for
- * multiple use locations, since a single definition location may overlap many use locations.
+ * Module containing the predicates that connect uses to their reaching definition. The reaching definitions are
+ * computed separately for each unique use `MemoryLocation`. An instruction is treated as a definition of a use location
+ * if the defined location overlaps the use location in any way. Thus, a single instruction may serve as a definition
+ * for multiple use locations, since a single definition location may overlap many use locations.
  * 
  * Definitions and uses are identified by a block and an integer "offset". An offset of -1 indicates the definition
  * from a `Phi` instruction at the beginning of the block. An offset of 2*i indicates a definition or use on the