Merge pull request github#3307 from dbartol/dbartol/BinaryConditional

C++: IR translation for binary conditional operator

Author: rdmarsh2

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

@@ -200,7 +200,11 @@ private predicate usedAsCondition(Expr expr) {
   or
   exists(IfStmt ifStmt | ifStmt.getCondition().getFullyConverted() = expr)
   or
-  exists(ConditionalExpr condExpr | condExpr.getCondition().getFullyConverted() = expr)
+  exists(ConditionalExpr condExpr |
+    // The two-operand form of `ConditionalExpr` treats its condition as a value, since it needs to
+    // be reused as a value if the condition is true.
+    condExpr.getCondition().getFullyConverted() = expr and not condExpr.isTwoOperand()
+  )
   or
   exists(NotExpr notExpr |
     notExpr.getOperand().getFullyConverted() = expr and

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

@@ -1758,20 +1758,20 @@ class TranslatedDestructorFieldDestruction extends TranslatedNonConstantExpr, St
   private TranslatedExpr getDestructorCall() { result = getTranslatedExpr(expr.getExpr()) }
 }
 
-class TranslatedConditionalExpr extends TranslatedNonConstantExpr, ConditionContext {
+/**
+ * The IR translation of the `?:` operator. This class has the portions of the implementation that
+ * are shared between the standard three-operand form (`a ? b : c`) and the GCC-extension
+ * two-operand form (`a ?: c`).
+ */
+abstract class TranslatedConditionalExpr extends TranslatedNonConstantExpr {
   override ConditionalExpr expr;
 
-  final override TranslatedElement getChild(int id) {
-    id = 0 and result = getCondition()
-    or
-    id = 1 and result = getThen()
-    or
-    id = 2 and result = getElse()
-  }
-
-  override Instruction getFirstInstruction() { result = getCondition().getFirstInstruction() }
-
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    // Note that the ternary flavor needs no explicit `ConditionalBranch` instruction here, because
+    // the condition is a `TranslatedCondition`, which will simply connect the successor edges of
+    // the condition directly to the appropriate then/else block via
+    // `getChild[True|False]Successor()`.
+    // The binary flavor will override this predicate to add the `ConditionalBranch`.
     not resultIsVoid() and
     (
       (
@@ -1866,13 +1866,13 @@ class TranslatedConditionalExpr extends TranslatedNonConstantExpr, ConditionCont
     )
   }
 
-  override predicate hasTempVariable(TempVariableTag tag, CppType type) {
+  final override predicate hasTempVariable(TempVariableTag tag, CppType type) {
     not resultIsVoid() and
     tag = ConditionValueTempVar() and
     type = getResultType()
   }
 
-  override IRVariable getInstructionVariable(InstructionTag tag) {
+  final override IRVariable getInstructionVariable(InstructionTag tag) {
     not resultIsVoid() and
     (
       tag = ConditionValueTrueTempAddressTag() or
@@ -1882,25 +1882,75 @@ class TranslatedConditionalExpr extends TranslatedNonConstantExpr, ConditionCont
     result = getTempVariable(ConditionValueTempVar())
   }
 
-  override Instruction getResult() {
+  final override Instruction getResult() {
     not resultIsVoid() and
     result = getInstruction(ConditionValueResultLoadTag())
   }
 
   override Instruction getChildSuccessor(TranslatedElement child) {
+    child = getElse() and
+    if elseIsVoid()
+    then result = getParent().getChildSuccessor(this)
+    else result = getInstruction(ConditionValueFalseTempAddressTag())
+  }
+
+  /**
+   * Gets the `TranslatedExpr` for the "then" result. Note that nothing in the base implementation
+   * of this class assumes that `getThen()` is disjoint from `getCondition()`.
+   */
+  abstract TranslatedExpr getThen();
+
+  /**
+   * Gets the `TranslatedExpr` for the "else" result.
+   */
+  final TranslatedExpr getElse() { result = getTranslatedExpr(expr.getElse().getFullyConverted()) }
+
+  final predicate thenIsVoid() {
+    getThen().getResultType().getIRType() instanceof IRVoidType
+    or
+    // A `ThrowExpr.getType()` incorrectly returns the type of exception being
+    // thrown, rather than `void`. Handle that case here.
+    expr.getThen() instanceof ThrowExpr
+  }
+
+  private predicate elseIsVoid() {
+    getElse().getResultType().getIRType() instanceof IRVoidType
+    or
+    // A `ThrowExpr.getType()` incorrectly returns the type of exception being
+    // thrown, rather than `void`. Handle that case here.
+    expr.getElse() instanceof ThrowExpr
+  }
+
+  private predicate resultIsVoid() { getResultType().getIRType() instanceof IRVoidType }
+}
+
+/**
+ * The IR translation of the ternary conditional operator (`a ? b : c`).
+ * For this version, we expand the condition as a `TranslatedCondition`, rather than a
+ * `TranslatedExpr`, to simplify the control flow in the presence of short-ciruit logical operators.
+ */
+class TranslatedTernaryConditionalExpr extends TranslatedConditionalExpr, ConditionContext {
+  TranslatedTernaryConditionalExpr() { not expr.isTwoOperand() }
+
+  final override TranslatedElement getChild(int id) {
+    id = 0 and result = getCondition()
+    or
+    id = 1 and result = getThen()
+    or
+    id = 2 and result = getElse()
+  }
+
+  override Instruction getFirstInstruction() { result = getCondition().getFirstInstruction() }
+
+  override Instruction getChildSuccessor(TranslatedElement child) {
+    result = TranslatedConditionalExpr.super.getChildSuccessor(child)
+    or
     (
       child = getThen() and
       if thenIsVoid()
       then result = getParent().getChildSuccessor(this)
       else result = getInstruction(ConditionValueTrueTempAddressTag())
     )
-    or
-    (
-      child = getElse() and
-      if elseIsVoid()
-      then result = getParent().getChildSuccessor(this)
-      else result = getInstruction(ConditionValueFalseTempAddressTag())
-    )
   }
 
   override Instruction getChildTrueSuccessor(TranslatedCondition child) {
@@ -1917,31 +1967,81 @@ class TranslatedConditionalExpr extends TranslatedNonConstantExpr, ConditionCont
     result = getTranslatedCondition(expr.getCondition().getFullyConverted())
   }
 
-  private TranslatedExpr getThen() {
+  final override TranslatedExpr getThen() {
     result = getTranslatedExpr(expr.getThen().getFullyConverted())
   }
+}
+
+/**
+ * The IR translation of a two-operand conditional operator (`a ?: b`). This is a GCC language
+ * extension.
+ * This version of the conditional expression returns its first operand (the condition) if that
+ * condition is non-zero. Since we'll be reusing the value of the condition, we'll compute that
+ * value directly before branching, even if that value was a short-circuit logical expression.
+ */
+class TranslatedBinaryConditionalExpr extends TranslatedConditionalExpr {
+  TranslatedBinaryConditionalExpr() { expr.isTwoOperand() }
 
-  private TranslatedExpr getElse() {
-    result = getTranslatedExpr(expr.getElse().getFullyConverted())
+  final override TranslatedElement getChild(int id) {
+    // We only truly have two children, because our "condition" and "then" are the same as far as
+    // the extractor is concerned.
+    id = 0 and result = getCondition()
+    or
+    id = 1 and result = getElse()
   }
 
-  private predicate thenIsVoid() {
-    getThen().getResultType().getIRType() instanceof IRVoidType
+  override Instruction getFirstInstruction() { result = getCondition().getFirstInstruction() }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    super.hasInstruction(opcode, tag, resultType)
     or
-    // A `ThrowExpr.getType()` incorrectly returns the type of exception being
-    // thrown, rather than `void`. Handle that case here.
-    expr.getThen() instanceof ThrowExpr
+    // For the binary variant, we create our own conditional branch.
+    tag = ValueConditionConditionalBranchTag() and
+    opcode instanceof Opcode::ConditionalBranch and
+    resultType = getVoidType()
   }
 
-  private predicate elseIsVoid() {
-    getElse().getResultType().getIRType() instanceof IRVoidType
+  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
+    result = super.getInstructionSuccessor(tag, kind)
     or
-    // A `ThrowExpr.getType()` incorrectly returns the type of exception being
-    // thrown, rather than `void`. Handle that case here.
-    expr.getElse() instanceof ThrowExpr
+    tag = ValueConditionConditionalBranchTag() and
+    (
+      kind instanceof TrueEdge and
+      result = getInstruction(ConditionValueTrueTempAddressTag())
+      or
+      kind instanceof FalseEdge and
+      result = getElse().getFirstInstruction()
+    )
   }
 
-  private predicate resultIsVoid() { getResultType().getIRType() instanceof IRVoidType }
+  override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
+    result = super.getInstructionOperand(tag, operandTag)
+    or
+    tag = ValueConditionConditionalBranchTag() and
+    operandTag instanceof ConditionOperandTag and
+    result = getCondition().getResult()
+  }
+
+  override Instruction getChildSuccessor(TranslatedElement child) {
+    result = super.getChildSuccessor(child)
+    or
+    child = getCondition() and result = getInstruction(ValueConditionConditionalBranchTag())
+  }
+
+  private TranslatedExpr getCondition() {
+    result = getTranslatedExpr(expr.getCondition().getFullyConverted())
+  }
+
+  final override TranslatedExpr getThen() {
+    // The extractor returns the exact same expression for `ConditionalExpr::getCondition()` and
+    // `ConditionalExpr::getThen()`, even though the condition may have been converted to `bool`,
+    // and the "then" may have been converted to the result type. We'll strip the top-level implicit
+    // conversions from this, to skip any conversion to `bool`. We don't have enough information to
+    // know how to convert the result to the destination type, especially in the class pointer case,
+    // so we'll still sometimes wind up with one operand as the wrong type. This is better than
+    // always converting the "then" operand to `bool`, which is almost always the wrong type.
+    result = getTranslatedExpr(expr.getThen().getExplicitlyConverted())
+  }
 }
 
 /**