No longer generate extra atoms to compare generic types (#655)

* switch max atoms to min atoms to avoid excess

* make type param info more available

* Provide typevars to constraint collection

* reset cache when typevar constraint is added

* trivial test changes

* Use constraint from TypeVarInfo in for malloc in possible

* Fix root cause output for constraint variables deleted from persistent constraints

* Fix failing array bounds tests

* special case for realloc

* include now-passing test in main test file

* Tweak change from 2a5db6f to also handle unsage allocator calls

* This fixes the 0 atoms affected root cause in test root_cause.c

* better comment

* Update clang/test/3C/root_cause.c

Co-authored-by: Aaron Eline <[email protected]>

* whitespace in test checked line

* Update clang/test/3C/hash.c

Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>

Co-authored-by: John Kastner <[email protected]>
Co-authored-by: Aaron Eline <[email protected]>
Co-authored-by: Matt McCutchen (Correct Computation) <[email protected]>

Author: 4 people

clang/include/clang/3C/3CInteractiveData.h

@@ -50,7 +50,7 @@ class ConstraintsInfo {
   CVars TotalNonDirectWildAtoms;
   CVars InSrcNonDirectWildAtoms;
   std::set<std::string> ValidSourceFiles;
-  std::map<ConstraintKey, const PersistentSourceLoc *> AtomSourceMap;
+  std::map<ConstraintKey, PersistentSourceLoc> AtomSourceMap;
 
 private:
   // Root cause map: This is the map of a Constraint var and a set of

clang/include/clang/3C/ProgramInfo.h

@@ -90,6 +90,7 @@ class ProgramInfo : public ProgramVariableAdder {
   // Store CVarSet with an empty set of BoundsKey into persistent contents.
   void storePersistentConstraints(clang::Expr *E, const CVarSet &Vars,
                                   ASTContext *C);
+  void removePersistentConstraints(Expr *E, ASTContext *C);
 
   // Get constraint variable for the provided Decl
   CVarOption getVariable(clang::Decl *D, clang::ASTContext *C);
@@ -169,6 +170,8 @@ class ProgramInfo : public ProgramVariableAdder {
   // expected that multiple entries will map to the same source location.
   std::map<IDAndTranslationUnit, PersistentSourceLoc> ExprLocations;
 
+  std::map<ConstraintKey, PersistentSourceLoc> DeletedAtomLocations;
+
   //Performance stats
   PerformanceStats PerfS;
 
@@ -210,8 +213,7 @@ class ProgramInfo : public ProgramVariableAdder {
   // Retrieves a FVConstraint* from a Decl (which could be static, or global)
   FVConstraint *getFuncFVConstraint(FunctionDecl *FD, ASTContext *C);
 
-  void insertIntoPtrSourceMap(const PersistentSourceLoc *PSL,
-                              ConstraintVariable *CV);
+  void insertIntoPtrSourceMap(PersistentSourceLoc PSL, ConstraintVariable *CV);
 
   void computePtrLevelStats();
 

clang/lib/3C/ConstraintBuilder.cpp

@@ -131,9 +131,8 @@ class InlineStructDetector {
 // and imposing constraints on variables it uses
 class FunctionVisitor : public RecursiveASTVisitor<FunctionVisitor> {
 public:
-  explicit FunctionVisitor(ASTContext *C, ProgramInfo &I, FunctionDecl *FD,
-                           TypeVarInfo &TVI)
-      : Context(C), Info(I), Function(FD), CB(Info, Context), TVInfo(TVI),
+  explicit FunctionVisitor(ASTContext *C, ProgramInfo &I, FunctionDecl *FD)
+      : Context(C), Info(I), Function(FD), CB(Info, Context),
         ISD() {}
 
   // T x = e
@@ -217,9 +216,10 @@ class FunctionVisitor : public RecursiveASTVisitor<FunctionVisitor> {
 
     // Collect type parameters for this function call that are
     // consistently instantiated as single type in this function call.
-    std::set<unsigned int> ConsistentTypeParams;
-    if (TFD != nullptr)
-      TVInfo.getConsistentTypeParams(E, ConsistentTypeParams);
+    auto ConsistentTypeParams =
+        Info.hasTypeParamBindings(E,Context) ?
+          Info.getTypeParamBindings(E,Context) :
+          ProgramInfo::CallTypeParamBindingsT();
 
     // Now do the call: Constrain arguments to parameters (but ignore returns)
     if (FVCons.empty()) {
@@ -436,7 +436,6 @@ class FunctionVisitor : public RecursiveASTVisitor<FunctionVisitor> {
   ProgramInfo &Info;
   FunctionDecl *Function;
   ConstraintResolver CB;
-  TypeVarInfo &TVInfo;
   InlineStructDetector ISD;
 };
 
@@ -485,9 +484,8 @@ class PtrToStructDef : public RecursiveASTVisitor<PtrToStructDef> {
 // for functions, variables, types, etc. that are visited.
 class ConstraintGenVisitor : public RecursiveASTVisitor<ConstraintGenVisitor> {
 public:
-  explicit ConstraintGenVisitor(ASTContext *Context, ProgramInfo &I,
-                                TypeVarInfo &TVI)
-      : Context(Context), Info(I), CB(Info, Context), TVInfo(TVI), ISD() {}
+  explicit ConstraintGenVisitor(ASTContext *Context, ProgramInfo &I)
+      : Context(Context), Info(I), CB(Info, Context), ISD() {}
 
   bool VisitVarDecl(VarDecl *G) {
 
@@ -526,7 +524,7 @@ class ConstraintGenVisitor : public RecursiveASTVisitor<ConstraintGenVisitor> {
     if (FL.isValid()) { // TODO: When would this ever be false?
       if (D->hasBody() && D->isThisDeclarationADefinition()) {
         Stmt *Body = D->getBody();
-        FunctionVisitor FV = FunctionVisitor(Context, Info, D, TVInfo);
+        FunctionVisitor FV = FunctionVisitor(Context, Info, D);
         FV.TraverseStmt(Body);
         if (AllTypes) {
           // Only do this, if all types is enabled.
@@ -551,7 +549,6 @@ class ConstraintGenVisitor : public RecursiveASTVisitor<ConstraintGenVisitor> {
   ASTContext *Context;
   ProgramInfo &Info;
   ConstraintResolver CB;
-  TypeVarInfo &TVInfo;
   InlineStructDetector ISD;
 };
 
@@ -669,7 +666,7 @@ void ConstraintBuilderConsumer::HandleTranslationUnit(ASTContext &C) {
   ConstraintResolver CSResolver(Info, &C);
   ContextSensitiveBoundsKeyVisitor CSBV =
       ContextSensitiveBoundsKeyVisitor(&C, Info, &CSResolver);
-  ConstraintGenVisitor GV = ConstraintGenVisitor(&C, Info, TV);
+  ConstraintGenVisitor GV = ConstraintGenVisitor(&C, Info);
   TranslationUnitDecl *TUD = C.getTranslationUnitDecl();
   StatsRecorder SR(&C, &Info);
 
@@ -681,13 +678,16 @@ void ConstraintBuilderConsumer::HandleTranslationUnit(ASTContext &C) {
 
     CSBV.TraverseDecl(D);
     TV.TraverseDecl(D);
-    GV.TraverseDecl(D);
-    SR.TraverseDecl(D);
   }
 
   // Store type variable information for use in rewriting
   TV.setProgramInfoTypeVars();
 
+  for (const auto &D : TUD->decls()) {
+    GV.TraverseDecl(D);
+    SR.TraverseDecl(D);
+  }
+
   if (Verbose)
     errs() << "Done analyzing\n";
 

clang/lib/3C/ConstraintResolver.cpp

@@ -151,6 +151,26 @@ inline CSetBkeyPair pairWithEmptyBkey(const CVarSet &Vars) {
   return std::make_pair(Vars, EmptyBSet);
 }
 
+// Get the return type of the function from the TypeVars, that is, from
+// the local instantiation of a generic function. Or the regular return
+// constraint if it's not generic
+ConstraintVariable *localReturnConstraint(
+    FVConstraint *FV,
+    ProgramInfo::CallTypeParamBindingsT TypeVars,
+    Constraints &CS) {
+  int TyVarIdx = FV->getExternalReturn()->getGenericIndex();
+  // Check if local type vars are available
+  if (TypeVars.find(TyVarIdx) != TypeVars.end() &&
+      TypeVars[TyVarIdx] != nullptr) {
+    ConstraintVariable *CV = TypeVars[TyVarIdx];
+    if (FV->getExternalReturn()->hasBoundsKey())
+      CV->setBoundsKey(FV->getExternalReturn()->getBoundsKey());
+    return CV;
+  } else {
+    return FV->getExternalReturn();
+  }
+}
+
 // Returns a pair of set of ConstraintVariables and set of BoundsKey
 // after evaluating the expression E. Will explore E recursively, but will
 // ignore parts of it that do not contribute to the final result.
@@ -403,6 +423,10 @@ CSetBkeyPair ConstraintResolver::getExprConstraintVars(Expr *E) {
       CVarSet ReturnCVs;
       BKeySet ReturnBSet = EmptyBSet;
 
+      ProgramInfo::CallTypeParamBindingsT TypeVars;
+      if (Info.hasTypeParamBindings(CE, Context))
+        TypeVars = Info.getTypeParamBindings(CE, Context);
+
       // Here, we need to look up the target of the call and return the
       // constraints for the return value of that function.
       QualType ExprType = E->getType();
@@ -418,22 +442,24 @@ CSetBkeyPair ConstraintResolver::getExprConstraintVars(Expr *E) {
 
         for (ConstraintVariable *C : Tmp.first) {
           if (FVConstraint *FV = dyn_cast<FVConstraint>(C)) {
-            ReturnCVs.insert(FV->getExternalReturn());
+            ReturnCVs.insert(localReturnConstraint(FV,TypeVars,CS));
           } else if (PVConstraint *PV = dyn_cast<PVConstraint>(C)) {
             if (FVConstraint *FV = PV->getFV())
-              ReturnCVs.insert(FV->getExternalReturn());
+              ReturnCVs.insert(localReturnConstraint(FV,TypeVars,CS));
           }
         }
       } else if (DeclaratorDecl *FD = dyn_cast<DeclaratorDecl>(D)) {
         /* Allocator call */
         if (isFunctionAllocator(std::string(FD->getName()))) {
           bool DidInsert = false;
           IsAllocator = true;
-          if (CE->getNumArgs() > 0) {
+          if (TypeVars.find(0) != TypeVars.end() && TypeVars[0] != nullptr) {
+            ReturnCVs.insert(TypeVars[0]);
+            DidInsert = true;
+          } else if (CE->getNumArgs() > 0) {
             QualType ArgTy;
             std::string FuncName = FD->getNameAsString();
-            ConstAtom *A;
-            A = analyzeAllocExpr(CE, CS, ArgTy, FuncName, Context);
+            ConstAtom *A = analyzeAllocExpr(CE, CS, ArgTy, FuncName, Context);
             if (A) {
               std::string N(FD->getName());
               N = "&" + N;
@@ -465,13 +491,13 @@ CSetBkeyPair ConstraintResolver::getExprConstraintVars(Expr *E) {
           assert(CV.hasValue() && "Function without constraint variable.");
           /* Direct function call */
           if (FVConstraint *FVC = dyn_cast<FVConstraint>(&CV.getValue()))
-            ReturnCVs.insert(FVC->getExternalReturn());
+            ReturnCVs.insert(localReturnConstraint(FVC,TypeVars,CS));
           /* Call via function pointer */
           else {
             PVConstraint *Tmp = dyn_cast<PVConstraint>(&CV.getValue());
             assert(Tmp != nullptr);
             if (FVConstraint *FVC = Tmp->getFV())
-              ReturnCVs.insert(FVC->getExternalReturn());
+              ReturnCVs.insert(localReturnConstraint(FVC,TypeVars,CS));
             else {
               // No FVConstraint -- make WILD.
               auto *TmpFV = new FVConstraint();

clang/lib/3C/ConstraintVariables.cpp

@@ -1786,10 +1786,11 @@ void constrainConsVarGeq(ConstraintVariable *LHS, ConstraintVariable *RHS,
 
         // Only generate constraint if LHS is not a base type.
         if (CLHS.size() != 0) {
-          if (CLHS.size() == CRHS.size() || PCLHS->getIsGeneric() ||
-              PCRHS->getIsGeneric()) {
-            unsigned Max = std::max(CLHS.size(), CRHS.size());
-            for (unsigned N = 0; N < Max; N++) {
+          if (CLHS.size() == CRHS.size() ||
+              (CLHS.size() < CRHS.size() && PCLHS->getIsGeneric()) ||
+              (CLHS.size() > CRHS.size() && PCRHS->getIsGeneric())) {
+            unsigned Min = std::min(CLHS.size(), CRHS.size());
+            for (unsigned N = 0; N < Min; N++) {
               Atom *IAtom = PCLHS->getAtom(N, CS);
               Atom *JAtom = PCRHS->getAtom(N, CS);
               if (IAtom == nullptr || JAtom == nullptr)
@@ -1931,20 +1932,8 @@ void PointerVariableConstraint::mergeDeclaration(ConstraintVariable *FromCV,
 }
 
 Atom *PointerVariableConstraint::getAtom(unsigned AtomIdx, Constraints &CS) {
-  if (AtomIdx < Vars.size()) {
-    // If index is in bounds, just return the atom.
-    return Vars[AtomIdx];
-  }
-  if (getIsGeneric() && AtomIdx == Vars.size()) {
-    // Polymorphic types don't know how "deep" their pointers are beforehand so,
-    // we need to create new atoms for new pointer levels on the fly.
-    std::string Stars(Vars.size(), '*');
-    Atom *A = CS.getFreshVar(Name + Stars, VarAtom::V_Other);
-    Vars.push_back(A);
-    SrcVars.push_back(CS.getWild());
-    return A;
-  }
-  return nullptr;
+  assert(AtomIdx < Vars.size());
+  return Vars[AtomIdx];
 }
 
 void PointerVariableConstraint::equateWithItype(

clang/lib/3C/ProgramInfo.cpp

@@ -829,6 +829,24 @@ void ProgramInfo::storePersistentConstraints(Expr *E, const CSetBkeyPair &Vars,
   ExprLocations[Key] = PSL;
 }
 
+void ProgramInfo::removePersistentConstraints(Expr *E, ASTContext *C) {
+  assert(hasPersistentConstraints(E, C) &&
+         "Persistent constraints not present.");
+
+  IDAndTranslationUnit Key = getExprKey(E, C);
+
+  // Save VarAtom locations so they can be used to assign source locations to
+  // root causes.
+  for (auto *CV : ExprConstraintVars[Key].first)
+    if (auto *PVC  = dyn_cast<PointerVariableConstraint>(CV))
+      for (Atom *A : PVC->getCvars())
+        if (auto *VA = dyn_cast<VarAtom>(A))
+          DeletedAtomLocations[VA->getLoc()] = ExprLocations[Key];
+
+  ExprConstraintVars.erase(Key);
+  ExprLocations.erase(Key);
+}
+
 // The Rewriter won't let us re-write things that are in macros. So, we
 // should check to see if what we just added was defined within a macro.
 // If it was, we should constrain it to top. This is sad. Hopefully,
@@ -1043,22 +1061,24 @@ bool ProgramInfo::computeInterimConstraintState(
   // Variables before ExprConstraintVars and making insertIntoPtrSourceMap not
   // overwrite a PSL already recorded for a given atom.
   for (const auto &I : Variables)
-    insertIntoPtrSourceMap(&(I.first), I.second);
+    insertIntoPtrSourceMap(I.first, I.second);
   for (const auto &I : ExprConstraintVars) {
-    PersistentSourceLoc *PSL = &ExprLocations[I.first];
+    PersistentSourceLoc PSL = ExprLocations[I.first];
     for (auto *J : I.second.first)
       insertIntoPtrSourceMap(PSL, J);
   }
+  for (auto E : DeletedAtomLocations)
+    CState.AtomSourceMap.insert(std::make_pair(E.first, E.second));
 
   auto &WildPtrsReason = CState.RootWildAtomsWithReason;
   for (auto *CurrC : CS.getConstraints()) {
     if (Geq *EC = dyn_cast<Geq>(CurrC)) {
       VarAtom *VLhs = dyn_cast<VarAtom>(EC->getLHS());
       if (EC->constraintIsChecked() && dyn_cast<WildAtom>(EC->getRHS())) {
         PersistentSourceLoc PSL = EC->getLocation();
-        const PersistentSourceLoc *APSL = CState.AtomSourceMap[VLhs->getLoc()];
-        if (!PSL.valid() && APSL && APSL->valid())
-          PSL = *APSL;
+        PersistentSourceLoc APSL = CState.AtomSourceMap[VLhs->getLoc()];
+        if (!PSL.valid() && APSL.valid())
+          PSL = APSL;
         WildPointerInferenceInfo Info(EC->getReason(), PSL);
         WildPtrsReason.insert(std::make_pair(VLhs->getLoc(), Info));
       }
@@ -1069,9 +1089,9 @@ bool ProgramInfo::computeInterimConstraintState(
   return true;
 }
 
-void ProgramInfo::insertIntoPtrSourceMap(const PersistentSourceLoc *PSL,
+void ProgramInfo::insertIntoPtrSourceMap(PersistentSourceLoc PSL,
                                          ConstraintVariable *CV) {
-  std::string FilePath = PSL->getFileName();
+  std::string FilePath = PSL.getFileName();
   if (canWrite(FilePath))
     CState.ValidSourceFiles.insert(FilePath);
 

clang/lib/3C/TypeVariableAnalysis.cpp

@@ -127,12 +127,27 @@ bool TypeVarVisitor::VisitCallExpr(CallExpr *CE) {
 
         // Constrain this variable GEQ the function arguments using the type
         // variable so if any of them are wild, the type argument will also be
-        // an unchecked pointer.
-        constrainConsVarGeq(P, TVEntry.second.getConstraintVariables(),
+        // an unchecked pointer. Except for realloc, which has special casing
+        // elsewhere, especially `ConstraintResolver::getExprConstraintVars`
+        // using variable `ReallocFlow`. Because `realloc` can take a wild
+        // pointer and return a safe one.
+        if (FD->getNameAsString() == "realloc") {
+          constrainConsVarGeq(P, TVEntry.second.getConstraintVariables(),
+                              Info.getConstraints(), nullptr, Wild_to_Safe, false,
+                              &Info);
+
+        } else {
+          constrainConsVarGeq(P, TVEntry.second.getConstraintVariables(),
                             Info.getConstraints(), nullptr, Safe_to_Wild, false,
                             &Info);
+      }
 
         TVEntry.second.setTypeParamConsVar(P);
+        // Since we've changed the constraint variable for this context, we
+        // need to remove the cache from the old one. Our new info will be
+        // used next request.
+        if (Info.hasPersistentConstraints(CE,Context))
+          Info.removePersistentConstraints(CE,Context);
       } else {
         // TODO: This might be too cautious.
         CR.constraintAllCVarsToWild(TVEntry.second.getConstraintVariables(),

clang/test/3C/hash.c

@@ -13,7 +13,10 @@ _Itype_for_any(T) void vsf_sysutil_memclr(void *p_dest
                                           : itype(_Array_ptr<T>)
                                                 byte_count(size),
                                             unsigned int size)
-// CHECK_ALL: _Itype_for_any(T) void vsf_sysutil_memclr(void *p_dest : itype(_Array_ptr<T>) byte_count(size), unsigned int size)
+// CHECK_ALL: _Itype_for_any(T) void vsf_sysutil_memclr(void *p_dest
+// CHECK_ALL-NEXT: : itype(_Array_ptr<T>)
+// CHECK_ALL-NEXT: byte_count(size),
+// CHECK_ALL-NEXT: unsigned int size)
 {
   /* Safety */
   if (size == 0) {

clang/test/3C/type_params.c

@@ -175,7 +175,7 @@ void *example1(void *ptr, unsigned int size) {
 // Issue #349. Check that the parameter doesn't inherit the double pointer
 // argument within do_doubleptr
 _Itype_for_any(T) void incoming_doubleptr(void *ptr : itype(_Array_ptr<T>)) {
-  // CHECK_ALL: _Itype_for_any(T) void incoming_doubleptr(void *ptr : itype(_Array_ptr<T>)) {
+  // CHECK_ALL: _Itype_for_any(T) void incoming_doubleptr(void *ptr : itype(_Array_ptr<T>) count(5)) {
   return;
 }
 
@@ -185,3 +185,12 @@ void do_doubleptr(int count) {
   incoming_doubleptr(arr);
   // CHECK_ALL: incoming_doubleptr<_Ptr<int>>(arr);
 }
+
+// make sure adding this function doesn't infer
+// with the type of the previous one
+// Though it does currently add the `count(5)`
+// to the param of incomming_doubleptr
+void interfere_doubleptr(void) {
+  float fl _Checked[5][5] = {};
+  incoming_doubleptr(fl);
+}