Handle function declaration appearing after definition (fix #399)

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)

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.

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);