Calls to polymorphic functions in macros no longer fail an assert (#480)

* change assert to consistency fail(macro)

* remove checked regions around inconsistent function calls; fix logic error

* add macro check

* don't add type params if they're all inconsistant

* add test

* remove commented code

* allow existing macro type params to be valid

* minor efficientcy

* use more appropriate rewrite check

clang/include/clang/3C/RewriteUtils.h

@@ -261,6 +261,8 @@ class RewriteConsumer : public ASTConsumer {
 
 bool canRewrite(Rewriter &R, const SourceRange &SR);
 
+bool canRewrite(clang::Expr &D, ASTContext &Context);
+
 // Rewrites the given source range with fallbacks for when the SourceRange is
 // inside a macro. This should be preferred to direct calls to ReplaceText
 // because this function will automatically expand macros where it needs to and

clang/include/clang/3C/TypeVariableAnalysis.h

@@ -20,11 +20,13 @@ class TypeVariableEntry {
 public:
   // Note: does not initialize TyVarType!
   TypeVariableEntry() : IsConsistent(false), TypeParamConsVar(nullptr) {}
-  TypeVariableEntry(QualType Ty, std::set<ConstraintVariable *> &CVs)
+  TypeVariableEntry(QualType Ty, std::set<ConstraintVariable *> &CVs
+                    , bool ForceInconsistent = false)
       : TypeParamConsVar(nullptr) {
     // We'll need a name to provide the type arguments during rewriting, so no
     // anonymous types are allowed.
-    IsConsistent = (Ty->isPointerType() || Ty->isArrayType()) &&
+    IsConsistent = !ForceInconsistent &&
+                   (Ty->isPointerType() || Ty->isArrayType()) &&
                    !isTypeAnonymous(Ty->getPointeeOrArrayElementType());
     TyVarType = Ty;
     ArgConsVars = CVs;
@@ -93,6 +95,10 @@ class TypeVarVisitor : public RecursiveASTVisitor<TypeVarVisitor>,
   ConstraintResolver CR;
   TypeVariableMapT TVMap;
 
-  void insertBinding(CallExpr *CE, const int TyIdx, QualType Ty, CVarSet &CVs);
+  void insertBinding(CallExpr *CE, const int TyIdx, QualType Ty,
+                     CVarSet &CVs, bool ForceInconsistent = false);
 };
+
+bool typeArgsProvided(CallExpr *Call);
+
 #endif

clang/lib/3C/CheckedRegions.cpp

@@ -224,6 +224,9 @@ bool CheckedRegionFinder::VisitCallExpr(CallExpr *C) {
     Map[ID] = isInStatementPosition(C) ? IS_CONTAINED : IS_UNCHECKED;
   } else {
     if (FD) {
+      if (Info.hasTypeParamBindings(C,Context))
+        for (auto Entry : Info.getTypeParamBindings(C, Context))
+          Wild |= (Entry.second == nullptr);
       auto Type = FD->getReturnType();
       Wild |= (!(FD->hasPrototype() || FD->doesThisDeclarationHaveABody())) ||
               containsUncheckedPtr(Type);

clang/lib/3C/ProgramInfo.cpp

@@ -1071,10 +1071,13 @@ void ProgramInfo::setTypeParamBinding(CallExpr *CE, unsigned int TypeVarIdx,
 
   auto PSL = PersistentSourceLoc::mkPSL(CE, *C);
   auto CallMap = TypeParamBindings[PSL];
-  assert("Attempting to overwrite type param binding in ProgramInfo." &&
-         CallMap.find(TypeVarIdx) == CallMap.end());
-
-  TypeParamBindings[PSL][TypeVarIdx] = CV;
+  if (CallMap.find(TypeVarIdx) == CallMap.end()) {
+    TypeParamBindings[PSL][TypeVarIdx] = CV;
+  } else {
+    // If this CE/idx is at the same location, it's in a macro,
+    // so mark it as inconsistent.
+    TypeParamBindings[PSL][TypeVarIdx] = nullptr;
+  }
 }
 
 bool ProgramInfo::hasTypeParamBindings(CallExpr *CE, ASTContext *C) const {

clang/lib/3C/RewriteUtils.cpp

@@ -14,6 +14,7 @@
 #include "clang/3C/CastPlacement.h"
 #include "clang/3C/CheckedRegions.h"
 #include "clang/3C/DeclRewriter.h"
+#include "clang/3C/TypeVariableAnalysis.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Tooling/Transformer/SourceCode.h"
 
@@ -400,8 +401,10 @@ class TypeArgumentAdder : public clang::RecursiveASTVisitor<TypeArgumentAdder> {
         // Construct a string containing concatenation of all type arguments for
         // the function call.
         std::string TypeParamString;
+        bool AllInconsistent = true;
         for (auto Entry : Info.getTypeParamBindings(CE, Context))
           if (Entry.second != nullptr) {
+            AllInconsistent = false;
             std::string TyStr = Entry.second->mkString(
                 Info.getConstraints().getVariables(), false, false, true);
             if (TyStr.back() == ' ')
@@ -414,8 +417,11 @@ class TypeArgumentAdder : public clang::RecursiveASTVisitor<TypeArgumentAdder> {
           }
         TypeParamString.pop_back();
 
-        SourceLocation TypeParamLoc = getTypeArgLocation(CE);
-        Writer.InsertTextAfter(TypeParamLoc, "<" + TypeParamString + ">");
+        // don't rewrite to malloc<void>(...), etc, just do malloc(...)
+        if (!AllInconsistent) {
+          SourceLocation TypeParamLoc = getTypeArgLocation(CE);
+          Writer.InsertTextAfter(TypeParamLoc, "<" + TypeParamString + ">");
+        }
       }
     }
     return true;
@@ -436,32 +442,6 @@ class TypeArgumentAdder : public clang::RecursiveASTVisitor<TypeArgumentAdder> {
     }
     llvm_unreachable("Could find SourceLocation for type arguments!");
   }
-
-  // Check if type arguments have already been provided for this function
-  // call so that we don't mess with anything already there.
-  bool typeArgsProvided(CallExpr *Call) {
-    Expr *Callee = Call->getCallee()->IgnoreImpCasts();
-    if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Callee)) {
-      // ArgInfo is null if there are no type arguments anywhere in the program
-      if (auto *ArgInfo = DRE->GetTypeArgumentInfo())
-        for (auto Arg : ArgInfo->typeArgumentss()) {
-          if (!Arg.typeName->isVoidType()) {
-            // Found a non-void type argument. No doubt type args are provided.
-            return true;
-          }
-          if (Arg.sourceInfo->getTypeLoc().getSourceRange().isValid()) {
-            // The type argument is void, but with a valid source range. This
-            // means an explict void type argument was provided.
-            return true;
-          }
-          // A void type argument without a source location. The type argument
-          // is implicit so, we're good to insert a new one.
-        }
-      return false;
-    }
-    // We only handle direct calls, so there must be a DeclRefExpr.
-    llvm_unreachable("Callee of function call is not DeclRefExpr.");
-  }
 };
 
 std::string ArrayBoundsRewriter::getBoundsString(const PVConstraint *PV,

clang/lib/3C/TypeVariableAnalysis.cpp

@@ -96,6 +96,9 @@ bool TypeVarVisitor::VisitCallExpr(CallExpr *CE) {
     if (FDef == nullptr)
       FDef = FD;
     if (auto *FVCon = Info.getFuncConstraint(FDef, Context)) {
+      // if we need to rewrite it but can't (macro, etc), it isn't safe
+      bool ForcedInconsistent = !typeArgsProvided(CE)
+                                && !Rewriter::isRewritable(CE->getExprLoc());
       // Visit each function argument, and if it use a type variable, insert it
       // into the type variable binding map.
       unsigned int I = 0;
@@ -107,7 +110,8 @@ bool TypeVarVisitor::VisitCallExpr(CallExpr *CE) {
         if (TyIdx >= 0) {
           Expr *Uncast = A->IgnoreImpCasts();
           std::set<ConstraintVariable *> CVs = CR.getExprConstraintVars(Uncast);
-          insertBinding(CE, TyIdx, Uncast->getType(), CVs);
+          insertBinding(CE, TyIdx, Uncast->getType(),
+                        CVs, ForcedInconsistent);
         }
         ++I;
       }
@@ -144,13 +148,14 @@ bool TypeVarVisitor::VisitCallExpr(CallExpr *CE) {
 // the exact type variable is identified by the call expression where it is
 // used and the index of the type variable type in the function declaration.
 void TypeVarVisitor::insertBinding(CallExpr *CE, const int TyIdx,
-                                   clang::QualType Ty, CVarSet &CVs) {
+                                   clang::QualType Ty, CVarSet &CVs,
+                                   bool ForceInconsistent) {
   assert(TyIdx >= 0 &&
          "Creating a type variable binding without a type variable.");
   auto &CallTypeVarMap = TVMap[CE];
   if (CallTypeVarMap.find(TyIdx) == CallTypeVarMap.end()) {
     // If the type variable hasn't been seen before, add it to the map.
-    TypeVariableEntry TVEntry = TypeVariableEntry(Ty, CVs);
+    TypeVariableEntry TVEntry = TypeVariableEntry(Ty, CVs, ForceInconsistent);
     CallTypeVarMap[TyIdx] = TVEntry;
   } else {
     // Otherwise, update entry with new type and constraints.
@@ -174,21 +179,41 @@ void TypeVarVisitor::getConsistentTypeParams(CallExpr *CE,
 // during rewriting.
 void TypeVarVisitor::setProgramInfoTypeVars() {
   for (const auto &TVEntry : TVMap) {
-    bool AllInconsistent = true;
+    // Add each type variable into the map in ProgramInfo. Inconsistent
+    // variables are mapped to null.
     for (auto TVCallEntry : TVEntry.second)
-      AllInconsistent &= !TVCallEntry.second.getIsConsistent();
-    // If they're all inconsistent type variables, ignore the call expression
-    if (!AllInconsistent) {
-      // Add each type variable into the map in ProgramInfo. Inconsistent
-      // variables are mapped to null.
-      for (auto TVCallEntry : TVEntry.second)
-        if (TVCallEntry.second.getIsConsistent())
-          Info.setTypeParamBinding(TVEntry.first, TVCallEntry.first,
-                                   TVCallEntry.second.getTypeParamConsVar(),
-                                   Context);
-        else
-          Info.setTypeParamBinding(TVEntry.first, TVCallEntry.first, nullptr,
-                                   Context);
-    }
+      if (TVCallEntry.second.getIsConsistent())
+        Info.setTypeParamBinding(TVEntry.first, TVCallEntry.first,
+                                 TVCallEntry.second.getTypeParamConsVar(),
+                                 Context);
+      else
+        Info.setTypeParamBinding(TVEntry.first, TVCallEntry.first, nullptr,
+                                 Context);
+  }
+}
+
+// Check if type arguments have already been provided for this function
+// call so that we don't mess with anything already there.
+bool typeArgsProvided(CallExpr *Call) {
+  Expr *Callee = Call->getCallee()->IgnoreImpCasts();
+  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Callee)) {
+    // ArgInfo is null if there are no type arguments anywhere in the program
+    if (auto *ArgInfo = DRE->GetTypeArgumentInfo())
+      for (auto Arg : ArgInfo->typeArgumentss()) {
+        if (!Arg.typeName->isVoidType()) {
+          // Found a non-void type argument. No doubt type args are provided.
+          return true;
+        }
+        if (Arg.sourceInfo->getTypeLoc().getSourceRange().isValid()) {
+          // The type argument is void, but with a valid source range. This
+          // means an explict void type argument was provided.
+          return true;
+        }
+        // A void type argument without a source location. The type argument
+        // is implicit so, we're good to insert a new one.
+      }
+    return false;
   }
+  // We only handle direct calls, so there must be a DeclRefExpr.
+  llvm_unreachable("Callee of function call is not DeclRefExpr.");
 }

clang/test/3C/type_params_macro.c

@@ -0,0 +1,48 @@
+// RUN: rm -rf %t*
+// RUN: 3c -base-dir=%S -addcr -alltypes %s -- | FileCheck -match-full-lines %s
+// RUN: 3c -base-dir=%S -addcr %s -- | FileCheck -match-full-lines %s
+// RUN: 3c -base-dir=%S -addcr %s -- | %clang -c -fcheckedc-extension -x c -o %t1.unused -
+
+// Test for invalid type arguments from a macro,
+// recognized also my checked regions
+
+#define baz foo<int>(i);
+#define buz foo(i);
+#define buzzy foo(i); foo(j);
+
+_Itype_for_any(T) void foo(void *x  : itype(_Ptr<T>));
+
+void test_none() {
+  int *i = 0;
+  foo(i);
+}
+// CHECK: void test_none() _Checked {
+// CHECK: _Ptr<int> i = 0;
+// CHECK: foo<int>(i);
+
+void test_one_given() {
+  int *i = 0;
+  baz
+}
+// CHECK: void test_one_given() _Checked {
+// CHECK: _Ptr<int> i = 0;
+
+void test_one() {
+  int *i = 0;
+  buz
+}
+// CHECK: void test_one() {
+// CHECK:  int *i = 0;
+// CHECK:  buz
+
+void test_two() {
+  int *i = 0;
+  int *j = 0;
+  buzzy
+}
+
+// CHECK: void test_two() {
+// CHECK:  int *i = 0;
+// CHECK:  int *j = 0;
+// CHECK:  buzzy
+