C++: IR translation for binary conditional operator

IR generation was not handling the special two-operand flavor of the `?:` operator that GCC supports as an extension. The extractor doesn't quite give us enough information to do this correctly (see github/codeql-c-extractor-team#67), but we can get pretty close.

About half of the code could be shared between the two-operand and three-operand flavors. The main differences for the two-operand flavor are:
1. The "then" operand isn't a child of the `ConditionalExpr`. Instead, we just reuse the original value of the "condition" operand, skipping any implicit cast to `bool` (see comment for rationale).
2. For the three-operand flavor, we generate the condition as control flow rather than the computation of a `bool` value, to avoid creating unnecessarily complicated branching. For the two-operand version, we just compute the value, since we have to reuse that value in the "then" branch anyway.

I've added IR tests for these new cases. I've also updated the expectations for `SignAnalysis.ql` based on the fix. @rdmarsh2, can you please double-check that these diffs look correct? I believe they do, but you're the range/sign analysis expert.

Author: Dave Bartolomeo

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,19 +1758,14 @@ 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) {
     not resultIsVoid() and
     (
@@ -1866,13 +1861,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 +1877,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 +1962,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())
+  }
 }
 
 /**