C++: Two IR fixes

My original fix in #1661 fixed my minimal test case, but did not fix the original failure in a Linux snapshot. The real fix is to simply not create a `TranslatedDeclarationEntry` for an extern declaration, and have `TranslatedDeclStmt` skip any such declarations. I've added a regression test for that case (multiple extern declarations with same location in a macro expansion, with control flow between them). I did verify that it generates correct IR, and that it fixes all of the "use not dominated by definition" failures in Linux.

The underlying extractor bug, that caused the above issue also caused PrintAST to print garbage. I've worked around the bug in PrintAST.qll.

I've also fixed a bug in the control flow for `try`/`catch`, where there was missing flow from the `CatchByType` of the last handler of a `try` to the enclosing handler (or `Unwind`). Hat tip to @andreidiaconu1 for spotting this bug.

Author: dave-bartolomeo

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/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()
     )
   }

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

@@ -7865,3 +7865,139 @@ ir.cpp:
 # 1118|   params: 
 # 1118|     0: [Parameter] p#0
 # 1118|         Type = [FloatType] float
+# 1128| [TopLevelFunction] void ExternDeclarationsInMacro()
+# 1128|   params: 
+# 1129|   body: [Block] { ... }
+# 1130|     0: [DeclStmt] declaration
+# 1130|       0: [VariableDeclarationEntry] declaration of g
+# 1130|           Type = [IntType] int
+# 1130|     1: [ForStmt] for(...;...;...) ...
+# 1130|       0: [DeclStmt] declaration
+# 1130|         0: [VariableDeclarationEntry] definition of i
+# 1130|             Type = [IntType] int
+# 1130|           init: [Initializer] initializer for i
+# 1130|             expr: [Literal,Zero] 0
+# 1130|                 Type = [IntType] int
+# 1130|                 Value = [Literal,Zero] 0
+# 1130|                 ValueCategory = prvalue
+# 1130|       1: [LTExpr] ... < ...
+# 1130|           Type = [BoolType] bool
+# 1130|           ValueCategory = prvalue
+# 1130|         0: [VariableAccess] i
+# 1130|             Type = [IntType] int
+# 1130|             ValueCategory = prvalue(load)
+# 1130|         1: [Literal] 10
+# 1130|             Type = [IntType] int
+# 1130|             Value = [Literal] 10
+# 1130|             ValueCategory = prvalue
+# 1130|       2: [PrefixIncrExpr] ++ ...
+# 1130|           Type = [IntType] int
+# 1130|           ValueCategory = lvalue
+# 1130|         0: [VariableAccess] i
+# 1130|             Type = [IntType] int
+# 1130|             ValueCategory = lvalue
+# 1130|       3: [Block] { ... }
+# 1130|         0: [DeclStmt] declaration
+# 1130|           0: [VariableDeclarationEntry] declaration of g
+# 1130|               Type = [IntType] int
+# 1130|     2: [EmptyStmt] ;
+# 1131|     3: [ReturnStmt] return ...
+# 1133| [TopLevelFunction] void TryCatchNoCatchAny(bool)
+# 1133|   params: 
+# 1133|     0: [Parameter] b
+# 1133|         Type = [BoolType] bool
+# 1133|   body: [Block] { ... }
+# 1134|     0: [TryStmt] try { ... }
+# 1134|       0: [Block] { ... }
+# 1135|         0: [DeclStmt] declaration
+# 1135|           0: [VariableDeclarationEntry] definition of x
+# 1135|               Type = [IntType] int
+# 1135|             init: [Initializer] initializer for x
+# 1135|               expr: [Literal] 5
+# 1135|                   Type = [IntType] int
+# 1135|                   Value = [Literal] 5
+# 1135|                   ValueCategory = prvalue
+# 1136|         1: [IfStmt] if (...) ... 
+# 1136|           0: [VariableAccess] b
+# 1136|               Type = [BoolType] bool
+# 1136|               ValueCategory = prvalue(load)
+# 1136|           1: [Block] { ... }
+# 1137|             0: [ExprStmt] ExprStmt
+# 1137|               0: [ThrowExpr] throw ...
+# 1137|                   Type = [PointerType] const char *
+# 1137|                   ValueCategory = prvalue
+# 1137|                 0: [ArrayToPointerConversion] array to pointer conversion
+# 1137|                     Type = [PointerType] const char *
+# 1137|                     ValueCategory = prvalue
+# 1137|                   expr: string literal
+# 1137|                       Type = [ArrayType] const char[15]
+# 1137|                       Value = [StringLiteral] "string literal"
+# 1137|                       ValueCategory = lvalue
+# 1139|           2: [IfStmt] if (...) ... 
+# 1139|             0: [LTExpr] ... < ...
+# 1139|                 Type = [BoolType] bool
+# 1139|                 ValueCategory = prvalue
+# 1139|               0: [VariableAccess] x
+# 1139|                   Type = [IntType] int
+# 1139|                   ValueCategory = prvalue(load)
+# 1139|               1: [Literal] 2
+# 1139|                   Type = [IntType] int
+# 1139|                   Value = [Literal] 2
+# 1139|                   ValueCategory = prvalue
+# 1139|             1: [Block] { ... }
+# 1140|               0: [ExprStmt] ExprStmt
+# 1140|                 0: [AssignExpr] ... = ...
+# 1140|                     Type = [IntType] int
+# 1140|                     ValueCategory = lvalue
+# 1140|                   0: [VariableAccess] x
+# 1140|                       Type = [IntType] int
+# 1140|                       ValueCategory = lvalue
+# 1140|                   1: [ConditionalExpr] ... ? ... : ...
+# 1140|                       Type = [IntType] int
+# 1140|                       ValueCategory = prvalue
+# 1140|                     0: [VariableAccess] b
+# 1140|                         Type = [BoolType] bool
+# 1140|                         ValueCategory = prvalue(load)
+# 1140|                     1: [Literal] 7
+# 1140|                         Type = [IntType] int
+# 1140|                         Value = [Literal] 7
+# 1140|                         ValueCategory = prvalue
+# 1140|                     2: [ThrowExpr] throw ...
+# 1140|                         Type = [Struct] String
+# 1140|                         ValueCategory = prvalue
+# 1140|                       0: [ConstructorCall] call to String
+# 1140|                           Type = [VoidType] void
+# 1140|                           ValueCategory = prvalue
+# 1140|                         0: [ArrayToPointerConversion] array to pointer conversion
+# 1140|                             Type = [PointerType] const char *
+# 1140|                             ValueCategory = prvalue
+# 1140|                           expr: String object
+# 1140|                               Type = [ArrayType] const char[14]
+# 1140|                               Value = [StringLiteral] "String object"
+# 1140|                               ValueCategory = lvalue
+# 1142|         2: [ExprStmt] ExprStmt
+# 1142|           0: [AssignExpr] ... = ...
+# 1142|               Type = [IntType] int
+# 1142|               ValueCategory = lvalue
+# 1142|             0: [VariableAccess] x
+# 1142|                 Type = [IntType] int
+# 1142|                 ValueCategory = lvalue
+# 1142|             1: [Literal] 7
+# 1142|                 Type = [IntType] int
+# 1142|                 Value = [Literal] 7
+# 1142|                 ValueCategory = prvalue
+# 1144|       1: [Handler] <handler>
+# 1144|         0: [CatchBlock] { ... }
+# 1145|           0: [ExprStmt] ExprStmt
+# 1145|             0: [ThrowExpr] throw ...
+# 1145|                 Type = [Struct] String
+# 1145|                 ValueCategory = prvalue
+# 1145|               0: [ConstructorCall] call to String
+# 1145|                   Type = [VoidType] void
+# 1145|                   ValueCategory = prvalue
+# 1145|                 0: [VariableAccess] s
+# 1145|                     Type = [PointerType] const char *
+# 1145|                     ValueCategory = prvalue(load)
+# 1147|       2: [Handler] <handler>
+# 1147|         0: [CatchBlock] { ... }
+# 1149|     1: [ReturnStmt] return ...