Merge pull request #3254 from dbartol/dbartol/ImplicitReturnValue2

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

Author: Robert Marsh

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,8 +131,11 @@ abstract class TranslatedReturnStmt extends TranslatedStmt {
   }
 }
 
+/**
+ * The IR translation of a `return` statement that returns a value.
+ */
 class TranslatedReturnValueStmt extends TranslatedReturnStmt, TranslatedVariableInitialization {
-  TranslatedReturnValueStmt() { stmt.hasExpr() }
+  TranslatedReturnValueStmt() { stmt.hasExpr() and hasReturnValue(stmt.getEnclosingFunction()) }
 
   final override Instruction getInitializationSuccessor() {
     result = getEnclosingFunction().getReturnSuccessorInstruction()
@@ -147,8 +150,49 @@ class TranslatedReturnValueStmt extends TranslatedReturnStmt, TranslatedVariable
   final override IRVariable getIRVariable() { result = getEnclosingFunction().getReturnVariable() }
 }
 
+/**
+ * The IR translation of a `return` statement that returns an expression of `void` type.
+ */
+class TranslatedReturnVoidExpressionStmt extends TranslatedReturnStmt {
+  TranslatedReturnVoidExpressionStmt() {
+    stmt.hasExpr() and not hasReturnValue(stmt.getEnclosingFunction())
+  }
+
+  override TranslatedElement getChild(int id) {
+    id = 0 and
+    result = getExpr()
+  }
+
+  override Instruction getFirstInstruction() { result = getExpr().getFirstInstruction() }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
+    tag = OnlyInstructionTag() and
+    opcode instanceof Opcode::NoOp and
+    resultType = getVoidType()
+  }
+
+  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
+    tag = OnlyInstructionTag() and
+    result = getEnclosingFunction().getReturnSuccessorInstruction() and
+    kind instanceof GotoEdge
+  }
+
+  override Instruction getChildSuccessor(TranslatedElement child) {
+    child = getExpr() and
+    result = getInstruction(OnlyInstructionTag())
+  }
+
+  private TranslatedExpr getExpr() { result = getTranslatedExpr(stmt.getExpr()) }
+}
+
+/**
+ * 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 +213,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,40 @@ 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 ...
+# 1295| [TopLevelFunction] void returnVoid(int, int)
+# 1295|   params: 
+# 1295|     0: [Parameter] x
+# 1295|         Type = [IntType] int
+# 1295|     1: [Parameter] y
+# 1295|         Type = [IntType] int
+# 1295|   body: [Block] { ... }
+# 1296|     0: [ReturnStmt] return ...
+# 1296|       0: [FunctionCall] call to IntegerOps
+# 1296|           Type = [VoidType] void
+# 1296|           ValueCategory = prvalue
+# 1296|         0: [VariableAccess] x
+# 1296|             Type = [IntType] int
+# 1296|             ValueCategory = prvalue(load)
+# 1296|         1: [VariableAccess] y
+# 1296|             Type = [IntType] int
+# 1296|             ValueCategory = prvalue(load)
 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,14 @@ 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;
+    }
+}
+
+void returnVoid(int x, int y) {
+    return IntegerOps(x, y);
+}
+
 // semmle-extractor-options: -std=c++17 --clang

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

@@ -6631,6 +6631,59 @@ 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             : 
+
+# 1295| void returnVoid(int, int)
+# 1295|   Block 0
+# 1295|     v1295_1(void)           = EnterFunction               : 
+# 1295|     mu1295_2(unknown)       = AliasedDefinition           : 
+# 1295|     mu1295_3(unknown)       = InitializeNonLocal          : 
+# 1295|     mu1295_4(unknown)       = UnmodeledDefinition         : 
+# 1295|     r1295_5(glval<int>)     = VariableAddress[x]          : 
+# 1295|     mu1295_6(int)           = InitializeParameter[x]      : &:r1295_5
+# 1295|     r1295_7(glval<int>)     = VariableAddress[y]          : 
+# 1295|     mu1295_8(int)           = InitializeParameter[y]      : &:r1295_7
+# 1296|     r1296_1(glval<unknown>) = FunctionAddress[IntegerOps] : 
+# 1296|     r1296_2(glval<int>)     = VariableAddress[x]          : 
+# 1296|     r1296_3(int)            = Load                        : &:r1296_2, ~mu1295_4
+# 1296|     r1296_4(glval<int>)     = VariableAddress[y]          : 
+# 1296|     r1296_5(int)            = Load                        : &:r1296_4, ~mu1295_4
+# 1296|     v1296_6(void)           = Call                        : func:r1296_1, 0:r1296_3, 1:r1296_5
+# 1296|     mu1296_7(unknown)       = ^CallSideEffect             : ~mu1295_4
+# 1296|     v1296_8(void)           = NoOp                        : 
+# 1295|     v1295_9(void)           = ReturnVoid                  : 
+# 1295|     v1295_10(void)          = UnmodeledUse                : mu*
+# 1295|     v1295_11(void)          = AliasedUse                  : ~mu1295_4
+# 1295|     v1295_12(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