Merge pull request #1674 from dave-bartolomeo/dave/ExternDecls2

C++: Two IR fixes and a PrintAST workaround

Author: jbj

cpp/ql/src/semmle/code/cpp/PrintAST.qll

@@ -67,11 +67,6 @@ private Function getEnclosingFunction(Locatable ast) {
   or
   result = ast.(Parameter).getFunction()
   or
-  exists(DeclStmt stmt |
-    stmt.getADeclarationEntry() = ast and
-    result = stmt.getEnclosingFunction()
-  )
-  or
   result = ast
 }
 
@@ -80,7 +75,14 @@ private Function getEnclosingFunction(Locatable ast) {
  * nodes for things like parameter lists and constructor init lists.
  */
 private newtype TPrintASTNode =
-  TASTNode(Locatable ast) { shouldPrintFunction(getEnclosingFunction(ast)) } or
+  TASTNode(Locatable ast) {
+    shouldPrintFunction(getEnclosingFunction(ast))
+  } or
+  TDeclarationEntryNode(DeclStmt stmt, DeclarationEntry entry) {
+    // We create a unique node for each pair of (stmt, entry), to avoid having one node with
+    // multiple parents due to extractor bug CPP-413.
+    stmt.getADeclarationEntry() = entry
+  } or
   TParametersNode(Function func) { shouldPrintFunction(func) } or
   TConstructorInitializersNode(Constructor ctor) {
     ctor.hasEntryPoint() and
@@ -161,11 +163,9 @@ private string qlClass(ElementBase el) { result = "[" + concat(el.getCanonicalQL
 /**
  * A node representing an AST node.
  */
-abstract class ASTNode extends PrintASTNode, TASTNode {
+abstract class BaseASTNode extends PrintASTNode {
   Locatable ast;
 
-  ASTNode() { this = TASTNode(ast) }
-
   override string toString() { result = qlClass(ast) + ast.toString() }
 
   final override Location getLocation() { result = getRepresentativeLocation(ast) }
@@ -176,6 +176,13 @@ abstract class ASTNode extends PrintASTNode, TASTNode {
   final Locatable getAST() { result = ast }
 }
 
+/**
+ * A node representing an AST node other than a `DeclarationEntry`.
+ */
+abstract class ASTNode extends BaseASTNode, TASTNode {
+  ASTNode() { this = TASTNode(ast) }
+}
+
 /**
  * A node representing an `Expr`.
  */
@@ -250,32 +257,33 @@ class CastNode extends ConversionNode {
 /**
  * A node representing a `DeclarationEntry`.
  */
-class DeclarationEntryNode extends ASTNode {
-  DeclarationEntry entry;
+class DeclarationEntryNode extends BaseASTNode, TDeclarationEntryNode {
+  override DeclarationEntry ast;
+  DeclStmt declStmt;
 
-  DeclarationEntryNode() { entry = ast }
+  DeclarationEntryNode() {
+    this = TDeclarationEntryNode(declStmt, ast)
+  }
 
   override PrintASTNode getChild(int childIndex) { none() }
 
   override string getProperty(string key) {
-    result = super.getProperty(key)
+    result = BaseASTNode.super.getProperty(key)
     or
     key = "Type" and
-    result = qlClass(entry.getType()) + entry.getType().toString()
+    result = qlClass(ast.getType()) + ast.getType().toString()
   }
 }
 
 /**
  * A node representing a `VariableDeclarationEntry`.
  */
 class VariableDeclarationEntryNode extends DeclarationEntryNode {
-  VariableDeclarationEntry varEntry;
-
-  VariableDeclarationEntryNode() { varEntry = entry }
+  override VariableDeclarationEntry ast;
 
   override ASTNode getChild(int childIndex) {
     childIndex = 0 and
-    result.getAST() = varEntry.getVariable().getInitializer()
+    result.getAST() = ast.getVariable().getInitializer()
   }
 
   override string getChildEdgeLabel(int childIndex) { childIndex = 0 and result = "init" }
@@ -289,7 +297,7 @@ class StmtNode extends ASTNode {
 
   StmtNode() { stmt = ast }
 
-  override ASTNode getChild(int childIndex) {
+  override BaseASTNode getChild(int childIndex) {
     exists(Locatable child |
       child = stmt.getChild(childIndex) and
       (
@@ -308,8 +316,11 @@ class DeclStmtNode extends StmtNode {
 
   DeclStmtNode() { declStmt = stmt }
 
-  override ASTNode getChild(int childIndex) {
-    result.getAST() = declStmt.getDeclarationEntry(childIndex)
+  override DeclarationEntryNode getChild(int childIndex) {
+    exists (DeclarationEntry entry |
+      declStmt.getDeclarationEntry(childIndex) = entry and
+      result = TDeclarationEntryNode(declStmt, entry)
+    )
   }
 }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll

@@ -215,6 +215,55 @@ module InstructionSanity {
       ) and
     fromInstr != fromBlock
   }
+
+  /**
+   * Gets the point in the function at which the specified operand is evaluated. For most operands,
+   * this is at the instruction that consumes the use. For a `PhiInputOperand`, the effective point
+   * of evaluation is at the end of the corresponding predecessor block.
+   */
+  private predicate pointOfEvaluation(Operand operand, IRBlock block, int index) {
+    (
+      block = operand.(PhiInputOperand).getPredecessorBlock() and
+      index = block.getInstructionCount()
+    ) or
+    exists (Instruction use |
+      use = operand.(NonPhiOperand).getUse() and
+      block.getInstruction(index) = use
+    )
+  }
+
+  /**
+   * Holds if `useOperand` has a definition that does not dominate the use.
+   */
+  query predicate useNotDominatedByDefinition(Operand useOperand, string message, IRFunction func,
+    string funcText) {
+
+    exists (IRBlock useBlock, int useIndex, Instruction defInstr, IRBlock defBlock, int defIndex |
+      not useOperand.getUse() instanceof UnmodeledUseInstruction and
+      pointOfEvaluation(useOperand, useBlock, useIndex) and
+      defInstr = useOperand.getAnyDef() and
+      (
+        (
+          defInstr instanceof PhiInstruction and
+          defBlock = defInstr.getBlock() and
+          defIndex = -1
+        )
+        or
+          defBlock.getInstruction(defIndex) = defInstr
+      ) and
+      not (
+        defBlock.strictlyDominates(useBlock) or
+        (
+          defBlock = useBlock and
+          defIndex < useIndex
+        )
+      ) and
+      message = "Operand '" + useOperand.toString() +
+        "' is not dominated by its definition in function '$@'." and
+      func = useOperand.getEnclosingIRFunction() and
+      funcText = Language::getIdentityString(func.getFunction())
+    )
+  }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll

@@ -215,6 +215,55 @@ module InstructionSanity {
       ) and
     fromInstr != fromBlock
   }
+
+  /**
+   * Gets the point in the function at which the specified operand is evaluated. For most operands,
+   * this is at the instruction that consumes the use. For a `PhiInputOperand`, the effective point
+   * of evaluation is at the end of the corresponding predecessor block.
+   */
+  private predicate pointOfEvaluation(Operand operand, IRBlock block, int index) {
+    (
+      block = operand.(PhiInputOperand).getPredecessorBlock() and
+      index = block.getInstructionCount()
+    ) or
+    exists (Instruction use |
+      use = operand.(NonPhiOperand).getUse() and
+      block.getInstruction(index) = use
+    )
+  }
+
+  /**
+   * Holds if `useOperand` has a definition that does not dominate the use.
+   */
+  query predicate useNotDominatedByDefinition(Operand useOperand, string message, IRFunction func,
+    string funcText) {
+
+    exists (IRBlock useBlock, int useIndex, Instruction defInstr, IRBlock defBlock, int defIndex |
+      not useOperand.getUse() instanceof UnmodeledUseInstruction and
+      pointOfEvaluation(useOperand, useBlock, useIndex) and
+      defInstr = useOperand.getAnyDef() and
+      (
+        (
+          defInstr instanceof PhiInstruction and
+          defBlock = defInstr.getBlock() and
+          defIndex = -1
+        )
+        or
+          defBlock.getInstruction(defIndex) = defInstr
+      ) and
+      not (
+        defBlock.strictlyDominates(useBlock) or
+        (
+          defBlock = useBlock and
+          defIndex < useIndex
+        )
+      ) and
+      message = "Operand '" + useOperand.toString() +
+        "' is not dominated by its definition in function '$@'." and
+      func = useOperand.getEnclosingIRFunction() and
+      funcText = Language::getIdentityString(func.getFunction())
+    )
+  }
 }
 
 /**

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

@@ -18,8 +18,8 @@ TranslatedDeclarationEntry getTranslatedDeclarationEntry(DeclarationEntry entry)
 /**
  * Represents the IR translation of a declaration within the body of a function.
  * Most often, this is the declaration of an automatic local variable, although
- * it can also be the declaration of a static local variable, an extern
- * variable, or an extern function.
+ * it can also be the declaration of a static local variable. Declarations of extern variables and
+ * functions do not have a `TranslatedDeclarationEntry`.
  */
 abstract class TranslatedDeclarationEntry extends TranslatedElement, TTranslatedDeclarationEntry {
   DeclarationEntry entry;
@@ -44,39 +44,6 @@ abstract class TranslatedDeclarationEntry extends TranslatedElement, TTranslated
   }
 }
 
-/**
- * Represents the IR translation of a declaration within the body of a function,
- * for declarations other than local variables. Since these have no semantic
- * effect, they do not generate any instructions.
- */
-class TranslatedNonVariableDeclarationEntry extends TranslatedDeclarationEntry {
-  TranslatedNonVariableDeclarationEntry() {
-    not entry.getDeclaration() instanceof LocalVariable
-  }
-
-  override predicate hasInstruction(Opcode opcode, InstructionTag tag,
-      Type resultType, boolean isGLValue) {
-    none()
-  }
-
-  override Instruction getFirstInstruction() {
-    result = getParent().getChildSuccessor(this)
-  }
-
-  override TranslatedElement getChild(int id) {
-    none()
-  }
-
-  override Instruction getInstructionSuccessor(InstructionTag tag,
-      EdgeKind kind) {
-    none()
-  }
-
-  override Instruction getChildSuccessor(TranslatedElement child) {
-    none()
-  }
-}
-
 /**
  * Represents the IR translation of the declaration of a local variable,
  * including its initialization, if any.

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

@@ -376,7 +376,9 @@ newtype TTranslatedElement =
   TTranslatedDeclarationEntry(DeclarationEntry entry) {
     exists(DeclStmt declStmt |
       translateStmt(declStmt) and
-      declStmt.getADeclarationEntry() = entry
+      declStmt.getADeclarationEntry() = entry and
+      // Only declarations of local variables need to be translated to IR.
+      entry.getDeclaration() instanceof LocalVariable
     )
   } or
   // A compiler-generated variable to implement a range-based for loop. These don't have a

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

@@ -69,6 +69,11 @@ class TranslatedEmptyStmt extends TranslatedStmt {
   }
 }
 
+/**
+ * The IR translation of a declaration statement. This consists of the IR for each of the individual
+ * local variables declared by the statement. Declarations for extern variables and functions
+ * do not generate any instructions.
+ */
 class TranslatedDeclStmt extends TranslatedStmt {
   override DeclStmt stmt;
 
@@ -82,15 +87,25 @@ class TranslatedDeclStmt extends TranslatedStmt {
   }
 
   override Instruction getFirstInstruction() {
-    result = getDeclarationEntry(0).getFirstInstruction() //REVIEW: Empty?
+    result = getDeclarationEntry(0).getFirstInstruction() or
+    not exists(getDeclarationEntry(0)) and result = getParent().getChildSuccessor(this)
   }
 
   private int getChildCount() {
-    result = stmt.getNumDeclarations()
+    result = count(getDeclarationEntry(_))
   }
 
+  /**
+   * Gets the `TranslatedDeclarationEntry` child at zero-based index `index`. Since not all
+   * `DeclarationEntry` objects have a `TranslatedDeclarationEntry` (e.g. extern functions), we map
+   * the original children into a contiguous range containing only those with an actual
+   * `TranslatedDeclarationEntry`.
+   */
   private TranslatedDeclarationEntry getDeclarationEntry(int index) {
-    result = getTranslatedDeclarationEntry(stmt.getDeclarationEntry(index))
+    result = rank[index + 1](TranslatedDeclarationEntry entry, int originalIndex |
+      entry = getTranslatedDeclarationEntry(stmt.getDeclarationEntry(originalIndex)) |
+      entry order by originalIndex
+    )
   }
 
   override Instruction getInstructionSuccessor(InstructionTag tag,
@@ -273,7 +288,7 @@ class TranslatedTryStmt extends TranslatedStmt {
       // The last catch clause flows to the exception successor of the parent
       // of the `try`, because the exception successor of the `try` itself is
       // the first catch clause.
-      handler = getHandler(stmt.getNumberOfCatchClauses()) and
+      handler = getHandler(stmt.getNumberOfCatchClauses() - 1) and
       result = getParent().getExceptionSuccessorInstruction()
     )
   }