C++: Treat implicit end of body of non-void function as Unreached

When the extractor can't prove that control flow will never reach the end of a non-`void`-returning function without reaching an explicit `return` statement, it inserts an implicit `return` without an operand. If control actually reaches this point, the behavior is undefined.

We were previously generating invalid IR for these implicit `return` statements, because the lack of an operand meant that there was no definition of the return value variable along that path. Instead, I've changed the IR generation to emit an `Unreached` instruction for the implicit `return`. This ensures that we don't create a control flow edge from the end of the body to the function epilogue.

The change to the range analysis test avoids having that test depend on the previous bad IR behavior, while still preserving the original spirit of the test.

Author: Dave Bartolomeo

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

@@ -49,6 +49,11 @@ CppType getEllipsisVariablePRValueType() {
 
 CppType getEllipsisVariableGLValueType() { result = getTypeForGLValue(any(UnknownType t)) }
 
+/**
+ * Holds if the function returns a value, as opposed to returning `void`.
+ */
+predicate hasReturnValue(Function func) { not func.getUnspecifiedType() instanceof VoidType }
+
 /**
  * Represents the IR translation of a function. This is the root elements for
  * all other elements associated with this function.
@@ -312,7 +317,7 @@ class TranslatedFunction extends TranslatedElement, TTranslatedFunction {
   /**
    * Holds if the function has a non-`void` return type.
    */
-  final predicate hasReturnValue() { not func.getUnspecifiedType() instanceof VoidType }
+  final predicate hasReturnValue() { hasReturnValue(func) }
 
   /**
    * Gets the single `UnmodeledDefinition` instruction for this function.

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

@@ -131,6 +131,9 @@ abstract class TranslatedReturnStmt extends TranslatedStmt {
   }
 }
 
+/**
+ * The IR translation of a `return` statement that returns a value.
+ */
 class TranslatedReturnValueStmt extends TranslatedReturnStmt, TranslatedVariableInitialization {
   TranslatedReturnValueStmt() { stmt.hasExpr() }
 
@@ -147,8 +150,14 @@ class TranslatedReturnValueStmt extends TranslatedReturnStmt, TranslatedVariable
   final override IRVariable getIRVariable() { result = getEnclosingFunction().getReturnVariable() }
 }
 
+/**
+ * The IR translation of a `return` statement that does not return a value. This includes implicit
+ * return statements at the end of `void`-returning functions.
+ */
 class TranslatedReturnVoidStmt extends TranslatedReturnStmt {
-  TranslatedReturnVoidStmt() { not stmt.hasExpr() }
+  TranslatedReturnVoidStmt() {
+    not stmt.hasExpr() and not hasReturnValue(stmt.getEnclosingFunction())
+  }
 
   override TranslatedElement getChild(int id) { none() }
 
@@ -169,6 +178,33 @@ class TranslatedReturnVoidStmt extends TranslatedReturnStmt {
   override Instruction getChildSuccessor(TranslatedElement child) { none() }
 }
 
+/**
+ * The IR translation of an implicit `return` statement generated by the extractor to handle control
+ * flow that reaches the end of a non-`void`-returning function body. Since such control flow
+ * produces undefined behavior, we simply generate an `Unreached` instruction to prevent that flow
+ * from continuing on to pollute other analysis. The assumption is that the developer is certain
+ * that the implicit `return` is unreachable, even if the compiler cannot prove it.
+ */
+class TranslatedUnreachableReturnStmt extends TranslatedReturnStmt {
+  TranslatedUnreachableReturnStmt() {
+    not stmt.hasExpr() and hasReturnValue(stmt.getEnclosingFunction())
+  }
+
+  override TranslatedElement getChild(int id) { none() }
+
+  override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    tag = OnlyInstructionTag() and
+    opcode instanceof Opcode::Unreached and
+    resultType = getVoidType()
+  }
+
+  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
+
+  override Instruction getChildSuccessor(TranslatedElement child) { none() }
+}
+
 /**
  * The IR translation of a C++ `try` statement.
  */

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

@@ -8750,6 +8750,23 @@ ir.cpp:
 # 1286|             Type = [PointerType] A *
 # 1286|             ValueCategory = prvalue
 # 1287|     12: [ReturnStmt] return ...
+# 1289| [TopLevelFunction] int missingReturnValue(bool, int)
+# 1289|   params: 
+# 1289|     0: [Parameter] b
+# 1289|         Type = [BoolType] bool
+# 1289|     1: [Parameter] x
+# 1289|         Type = [IntType] int
+# 1289|   body: [Block] { ... }
+# 1290|     0: [IfStmt] if (...) ... 
+# 1290|       0: [VariableAccess] b
+# 1290|           Type = [BoolType] bool
+# 1290|           ValueCategory = prvalue(load)
+# 1290|       1: [Block] { ... }
+# 1291|         0: [ReturnStmt] return ...
+# 1291|           0: [VariableAccess] x
+# 1291|               Type = [IntType] int
+# 1291|               ValueCategory = prvalue(load)
+# 1293|     1: [ReturnStmt] return ...
 perf-regression.cpp:
 #    4| [CopyAssignmentOperator] Big& Big::operator=(Big const&)
 #    4|   params: 

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

@@ -1249,10 +1249,10 @@ char *strcpy(char *destination, const char *source);
 char *strcat(char *destination, const char *source);
 
 void test_strings(char *s1, char *s2) {
-	char buffer[1024] = {0};
+    char buffer[1024] = {0};
 
-	strcpy(buffer, s1);
-	strcat(buffer, s2);
+    strcpy(buffer, s1);
+    strcat(buffer, s2);
 }
 
 struct A {
@@ -1286,4 +1286,10 @@ void test_static_member_functions(int int_arg, A* a_arg) {
     getAnInstanceOfA()->static_member_without_def();
 }
 
+int missingReturnValue(bool b, int x) {
+    if (b) {
+        return x;
+    }
+}
+
 // semmle-extractor-options: -std=c++17 --clang

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

@@ -6631,6 +6631,36 @@ ir.cpp:
 # 1270|     v1270_14(void)          = AliasedUse                                 : ~mu1270_4
 # 1270|     v1270_15(void)          = ExitFunction                               : 
 
+# 1289| int missingReturnValue(bool, int)
+# 1289|   Block 0
+# 1289|     v1289_1(void)        = EnterFunction          : 
+# 1289|     mu1289_2(unknown)    = AliasedDefinition      : 
+# 1289|     mu1289_3(unknown)    = InitializeNonLocal     : 
+# 1289|     mu1289_4(unknown)    = UnmodeledDefinition    : 
+# 1289|     r1289_5(glval<bool>) = VariableAddress[b]     : 
+# 1289|     mu1289_6(bool)       = InitializeParameter[b] : &:r1289_5
+# 1289|     r1289_7(glval<int>)  = VariableAddress[x]     : 
+# 1289|     mu1289_8(int)        = InitializeParameter[x] : &:r1289_7
+# 1290|     r1290_1(glval<bool>) = VariableAddress[b]     : 
+# 1290|     r1290_2(bool)        = Load                   : &:r1290_1, ~mu1289_4
+# 1290|     v1290_3(void)        = ConditionalBranch      : r1290_2
+#-----|   False -> Block 1
+#-----|   True -> Block 2
+
+# 1293|   Block 1
+# 1293|     v1293_1(void) = Unreached : 
+
+# 1291|   Block 2
+# 1291|     r1291_1(glval<int>) = VariableAddress[#return] : 
+# 1291|     r1291_2(glval<int>) = VariableAddress[x]       : 
+# 1291|     r1291_3(int)        = Load                     : &:r1291_2, ~mu1289_4
+# 1291|     mu1291_4(int)       = Store                    : &:r1291_1, r1291_3
+# 1289|     r1289_9(glval<int>) = VariableAddress[#return] : 
+# 1289|     v1289_10(void)      = ReturnValue              : &:r1289_9, ~mu1289_4
+# 1289|     v1289_11(void)      = UnmodeledUse             : mu*
+# 1289|     v1289_12(void)      = AliasedUse               : ~mu1289_4
+# 1289|     v1289_13(void)      = ExitFunction             : 
+
 perf-regression.cpp:
 #    6| void Big::Big()
 #    6|   Block 0

cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected

@@ -35,7 +35,7 @@
 | test.cpp:97:10:97:10 | Load: x | file://:0:0:0:0 | 0 | 1 | false | CompareLT: ... < ... | test.cpp:94:7:94:11 | test.cpp:94:7:94:11 |
 | test.cpp:100:10:100:10 | Load: x | file://:0:0:0:0 | 0 | 1 | true | CompareLE: ... <= ... | test.cpp:99:7:99:12 | test.cpp:99:7:99:12 |
 | test.cpp:102:10:102:10 | Load: x | file://:0:0:0:0 | 0 | 2 | false | CompareLE: ... <= ... | test.cpp:99:7:99:12 | test.cpp:99:7:99:12 |
-| test.cpp:107:5:107:10 | Phi: test10 | test.cpp:114:3:114:6 | Phi: call to sink | -1 | true | CompareLT: ... < ... | test.cpp:115:18:115:22 | test.cpp:115:18:115:22 |
+| test.cpp:117:10:117:10 | Load: i | test.cpp:114:3:114:6 | Phi: call to sink | -1 | true | CompareLT: ... < ... | test.cpp:116:7:116:11 | test.cpp:116:7:116:11 |
 | test.cpp:130:10:130:10 | Load: i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
 | test.cpp:140:10:140:10 | Store: i | file://:0:0:0:0 | 0 | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 |
 | test.cpp:140:10:140:10 | Store: i | test.cpp:135:16:135:16 | InitializeParameter: x | 0 | false | CompareLT: ... < ... | test.cpp:139:11:139:15 | test.cpp:139:11:139:15 |

cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp

@@ -104,17 +104,17 @@ void test9(int x) {
 }
 
 // Phi nodes as bounds
-int test10(int y, int z, bool use_y) {
+void test10(int y, int z, bool use_y) {
   int x;
   if(use_y) {
     x = y;
   } else {
     x = z;
   }
   sink();
-  for(int i = 0; i < x; i++) {
-    return i;
-  }
+  int i = source();
+  if (i < x)
+    sink(i);
 }
 
 // Irreducible CFGs