Handle function declaration appearing after definition (fix #399) (#411)

john-h-kastner · web-flow · commit 785f962bb45c · 2021-02-08T12:41:45.000-05:00
Ensure that constrains generated due to a function's declaration are
applied to the variables generated at the definition even when
declaration appears after the definition. This is done by copying
the atoms from the definition constraint variable into the declaration
constraint variable using brainTransplant before any constraints are
added on the declaration.

Also change insertNewFVConstraint to return void. Previously, it
returned a bool, but the value was never used. This also
applies to two functions that are only ever called from inside
insertNewFVConstraint.
diff --git a/clang/include/clang/3C/ProgramInfo.h b/clang/include/clang/3C/ProgramInfo.h
@@ -169,15 +169,13 @@ class ProgramInfo : public ProgramVariableAdder {
   TypeParamBindingsT TypeParamBindings;
 
   // Insert the given FVConstraint* set into the provided Map.
-  // Returns true if successful else false.
-  bool insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
+  void insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
                                      const std::string &FuncName,
-                                     FVConstraint *ToIns, FunctionDecl *FD,
+                                     FVConstraint *NewC, FunctionDecl *FD,
                                      ASTContext *C);
 
   // Inserts the given FVConstraint* set into the provided static map.
-  // Returns true if successful else false.
-  bool insertIntoStaticFunctionMap(StaticFunctionMapType &Map,
+  void insertIntoStaticFunctionMap(StaticFunctionMapType &Map,
                                    const std::string &FuncName,
                                    const std::string &FileName,
                                    FVConstraint *ToIns, FunctionDecl *FD,
@@ -188,9 +186,14 @@ class ProgramInfo : public ProgramVariableAdder {
   //  * va_list-typed variables
   void specialCaseVarIntros(ValueDecl *D, ASTContext *Context);
 
-  // Inserts the given FVConstraint* set into the global map, depending
-  // on whether static or not; returns true on success
-  bool insertNewFVConstraint(FunctionDecl *FD, FVConstraint *FVCon,
+  // Inserts the given FVConstraint set into the extern or static function map.
+  // Note: This can trigger a brainTransplant from an existing FVConstraint into
+  // the argument FVConstraint. The brainTransplant copies the atoms of the
+  // existing FVConstraint into the argument. This effectively throws out any
+  // constraints that may been applied to the argument FVConstraint, so do not
+  // call this function any time other than immediately after constructing an
+  // FVConstraint.
+  void insertNewFVConstraint(FunctionDecl *FD, FVConstraint *FVCon,
                              ASTContext *C);
 
   // Retrieves a FVConstraint* from a Decl (which could be static, or global)
diff --git a/clang/lib/3C/ProgramInfo.cpp b/clang/lib/3C/ProgramInfo.cpp
@@ -402,23 +402,20 @@ void ProgramInfo::exitCompilationUnit() {
   return;
 }
 
-bool ProgramInfo::insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
+void ProgramInfo::insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
                                                 const std::string &FuncName,
                                                 FVConstraint *NewC,
                                                 FunctionDecl *FD,
                                                 ASTContext *C) {
-  bool RetVal = false;
   if (Map.find(FuncName) == Map.end()) {
     Map[FuncName] = NewC;
-    RetVal = true;
   } else {
     auto *OldC = Map[FuncName];
     if (!OldC->hasBody()) {
       if (NewC->hasBody() ||
           (OldC->numParams() == 0 && NewC->numParams() != 0)) {
         NewC->brainTransplant(OldC, *this);
         Map[FuncName] = NewC;
-        RetVal = true;
       } else {
         // if the current FV constraint is not a definition?
         // then merge.
@@ -450,44 +447,42 @@ bool ProgramInfo::insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
           DiagnosticsEngine::Fatal, "duplicate definition for function %0");
       DE.Report(FD->getLocation(), DuplicateDefinitionsID).AddString(FuncName);
       exit(1);
+    } else {
+      // The old constraint has a body, but we've encountered another prototype
+      // for the function.
+      assert(OldC->hasBody() && !NewC->hasBody());
+      // By transplanting the atoms of OldC into NewC, we ensure that any
+      // constraints applied to NewC later on constrain the atoms of OldC.
+      NewC->brainTransplant(OldC, *this);
     }
-    // else oldc->hasBody() so we can ignore the newC prototype
   }
-  return RetVal;
 }
 
-bool ProgramInfo::insertIntoStaticFunctionMap(StaticFunctionMapType &Map,
+void ProgramInfo::insertIntoStaticFunctionMap(StaticFunctionMapType &Map,
                                               const std::string &FuncName,
                                               const std::string &FileName,
                                               FVConstraint *ToIns,
                                               FunctionDecl *FD, ASTContext *C) {
-  bool RetVal = false;
-  if (Map.find(FileName) == Map.end()) {
+  if (Map.find(FileName) == Map.end())
     Map[FileName][FuncName] = ToIns;
-    RetVal = true;
-  } else {
-    RetVal =
-        insertIntoExternalFunctionMap(Map[FileName], FuncName, ToIns, FD, C);
-  }
-  return RetVal;
+  else
+    insertIntoExternalFunctionMap(Map[FileName], FuncName, ToIns, FD, C);
 }
 
-bool ProgramInfo::insertNewFVConstraint(FunctionDecl *FD, FVConstraint *FVCon,
+void ProgramInfo::insertNewFVConstraint(FunctionDecl *FD, FVConstraint *FVCon,
                                         ASTContext *C) {
-  bool Ret = false;
   std::string FuncName = FD->getNameAsString();
   if (FD->isGlobal()) {
     // external method.
-    Ret = insertIntoExternalFunctionMap(ExternalFunctionFVCons, FuncName, FVCon,
-                                        FD, C);
+    insertIntoExternalFunctionMap(ExternalFunctionFVCons, FuncName, FVCon, FD,
+                                  C);
   } else {
     // static method
     auto Psl = PersistentSourceLoc::mkPSL(FD, *C);
     std::string FuncFileName = Psl.getFileName();
-    Ret = insertIntoStaticFunctionMap(StaticFunctionFVCons, FuncName,
-                                      FuncFileName, FVCon, FD, C);
+    insertIntoStaticFunctionMap(StaticFunctionFVCons, FuncName, FuncFileName,
+                                FVCon, FD, C);
   }
-  return Ret;
 }
 
 void ProgramInfo::specialCaseVarIntros(ValueDecl *D, ASTContext *Context) {
@@ -548,32 +543,39 @@ void ProgramInfo::addVariable(clang::DeclaratorDecl *D,
     // Function Decls have FVConstraints.
     FVConstraint *F = new FVConstraint(D, *this, *AstContext);
     F->setValidDecl();
-    PVConstraint *RetExternal = F->getExternalReturn();
-    PVConstraint *RetInternal = F->getInternalReturn();
-    auto Ret_Ty = FD->getReturnType();
-    unifyIfTypedef(Ret_Ty.getTypePtr(), *AstContext, FD, RetExternal);
-    unifyIfTypedef(Ret_Ty.getTypePtr(), *AstContext, FD, RetInternal);
-
 
     // Handling of PSL collision for functions is different since we need to
     // consider the static and extern function maps.
     if (Variables.find(PLoc) != Variables.end()) {
       // Try to find a previous definition based on function name
       if (!getFuncConstraint(FD, AstContext)) {
-        constrainWildIfMacro(F, FD->getLocation());
+        // No function with the same name exists. It's concerning that
+        // something already exists at this source location, but we add the
+        // function to the function map anyways. The function map indexes by
+        // function name, so there's no collision.
         insertNewFVConstraint(FD, F, AstContext);
+        constrainWildIfMacro(F, FD->getLocation());
       } else {
-        // FIXME: Visiting same function in same source location twice.
-        //        This shouldn't happen, but it does for some std lib functions
-        //        on our benchmarks programs (vsftpd, lua, etc.). Bailing for
-        //        now, but a real fix will catch and prevent this earlier.
+        // A function with the same name exists in the same source location.
+        // This happens when a function is defined in a header file which is
+        // included in multiple translation units. getFuncConstraint returned
+        // non-null, so we know that the definition has been processed already,
+        // and there is no more work to do.
       }
       return;
     }
 
-    /* Store the FVConstraint in the global and Variables maps */
+    // Store the FVConstraint in the global and Variables maps. In doing this,
+    // insertNewFVConstraint might replace the atoms in F with the atoms of a
+    // FVConstraint that already exists in the map. Doing this loses any
+    // constraints that might have effected the original atoms, so do not create
+    // any constraint on F before this function is called.
     insertNewFVConstraint(FD, F, AstContext);
 
+    auto RetTy = FD->getReturnType();
+    unifyIfTypedef(RetTy.getTypePtr(), *AstContext, FD, F->getExternalReturn());
+    unifyIfTypedef(RetTy.getTypePtr(), *AstContext, FD, F->getInternalReturn());
+
     NewCV = F;
     // Add mappings from the parameters PLoc to the constraint variables for
     // the parameters.
diff --git a/clang/test/3C/defn_then_decl.c b/clang/test/3C/defn_then_decl.c
@@ -0,0 +1,37 @@
+// RUN: 3c -alltypes -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s
+// RUN: 3c -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_NOALL","CHECK" %s
+// RUN: 3c -alltypes -addcr %s -- | %clang -c -fcheckedc-extension -x c -o /dev/null -
+// RUN: 3c -alltypes -output-postfix=checked %s
+// RUN: 3c -alltypes %S/defn_then_decl.checked.c -- | count 0
+// RUN: rm %S/defn_then_decl.checked.c
+
+// Tests behavior when a function declaration appears after the definition for
+// that function. A specific issue that existed caused constraints applied to
+// the declaration (for example, when the declaration is in a macro) to not
+// be correctly applied to the original definition.
+
+void test0(int *p) {}
+#define MYDECL void test0(int *p);
+MYDECL
+
+void test1(int *p) {}
+//CHECK: void test1(_Ptr<int> p) _Checked {}
+void test1(int *p);
+//CHECK: void test1(_Ptr<int> p);
+
+void test2(int *p);
+//CHECK: void test2(_Ptr<int> p);
+void test2(int *p) {}
+//CHECK: void test2(_Ptr<int> p) _Checked {}
+void test2(int *p);
+//CHECK: void test2(_Ptr<int> p);
+
+void test3(int *p) { p = 1; }
+//CHECK: void test3(int *p : itype(_Ptr<int>)) { p = 1; }
+void test3(int *p);
+//CHECK: void test3(int *p : itype(_Ptr<int>));
+
+void test4(int *p) {}
+//CHECK: void test4(_Ptr<int> p) _Checked {}
+void test4();
+//CHECK: void test4(_Ptr<int> p);
