Merge pull request #254 from plum-umd/ImproveMacroConversion

Improve macro conversion

clang/include/clang/CConv/RewriteUtils.h

@@ -28,7 +28,7 @@ class DeclReplacement {
 
   std::string getReplacement() const { return Replacement; }
 
-  virtual SourceRange getSourceRange(SourceManager &SR) const {
+  virtual SourceRange getSourceRange() const {
     return getDecl()->getSourceRange();
   }
 
@@ -85,24 +85,56 @@ class FunctionDeclReplacement :
     public DeclReplacementTempl<FunctionDecl,
                                 DeclReplacement::DRK_FunctionDecl> {
 public:
-  explicit FunctionDeclReplacement(FunctionDecl *D, std::string R, bool Full)
-      : DeclReplacementTempl(D, nullptr, R), FullDecl(Full) {}
-
-  SourceRange getSourceRange(SourceManager &SM) const override {
-    if (FullDecl) {
-      SourceRange Range = Decl->getSourceRange();
-      Range.setEnd(getFunctionDeclarationEnd(Decl, SM));
-      return Range;
-    } else
-      return Decl->getReturnTypeSourceRange();
+  explicit FunctionDeclReplacement(FunctionDecl *D, std::string R, bool Return,
+                                   bool Params)
+      : DeclReplacementTempl(D, nullptr, R), RewriteReturn(Return),
+        RewriteParams(Params) {
+    assert("Doesn't make sense to rewrite nothing!"
+           && (RewriteReturn || RewriteParams));
   }
 
-  bool isFullDecl() const {
-    return FullDecl;
+  SourceRange getSourceRange() const override {
+    FunctionTypeLoc TypeLoc =
+        Decl->getTypeSourceInfo()->getTypeLoc().getAs<clang::FunctionTypeLoc>();
+
+    // Function pointer are funky, and require special handling to rewrite the
+    // return type.
+    if (Decl->getReturnType()->isFunctionPointerType()){
+      if (RewriteParams && RewriteReturn) {
+        SourceLocation End = TypeLoc.getReturnLoc().getNextTypeLoc()
+                                    .getAs<clang::ParenTypeLoc>().getInnerLoc()
+                                    .getAs<clang::FunctionTypeLoc>()
+                                    .getRParenLoc();
+        return SourceRange(Decl->getBeginLoc(), End);
+      }
+      assert("RewriteReturn implies RewriteParams for function pointer return."
+             && !RewriteReturn);
+      // Fall through to standard handling when only rewriting param decls
+    }
+
+    // If rewriting the return, then the range starts at the begining of the
+    // decl. Otherwise, skip to the left parenthesis of parameters.
+    SourceLocation Begin = RewriteReturn ?
+        Decl->getBeginLoc() :
+        TypeLoc.getLParenLoc();
+
+    // If rewriting Parameters, stop at the right parenthesis of the parameters.
+    // Otherwise, stop after the return type.
+    SourceLocation End = RewriteParams ?
+        TypeLoc.getRParenLoc() :
+        Decl->getReturnTypeSourceRange().getEnd();
+
+    assert("Invalid FunctionDeclReplacement SourceRange!"
+           && Begin.isValid() && End.isValid());
+
+    return SourceRange(Begin, End);
   }
+
 private:
   // This determines if the full declaration or the return will be replaced.
-  bool FullDecl;
+  bool RewriteReturn;
+
+  bool RewriteParams;
 };
 
 // Compare two DeclReplacement values. The algorithm for comparing them relates

clang/include/clang/CConv/Utils.h

@@ -162,7 +162,7 @@ bool evaluateToInt(clang::Expr *E, const clang::ASTContext &C, int &Result);
 // can be treated as pointers.
 bool isZeroBoundsExpr(clang::BoundsExpr *BE, const clang::ASTContext &C);
 
-// Get the FileIdD for the file containing the given source location. This is
-// used to maintain a set of FileIDs which have been modified by the rewriter.
-clang::FileID getFileID(clang::SourceLocation L, clang::ASTContext &C);
+// Find the range in the source code for the base type of a type location.
+// The base type is the type after removing all
+clang::TypeLoc getBaseTypeLoc(clang::TypeLoc T);
 #endif

clang/lib/CConv/ConstraintVariables.cpp

@@ -150,7 +150,7 @@ PointerVariableConstraint::PointerVariableConstraint(const QualType &QT,
 
   ArrPresent = false;
 
-  bool isDeclTy = false;
+  bool IsDeclTy = false;
   if (D != nullptr) {
     auto &ABInfo = I.getABoundsInfo();
     if (ABInfo.tryGetVariable(D, BKey)) {
@@ -174,14 +174,14 @@ PointerVariableConstraint::PointerVariableConstraint(const QualType &QT,
       }
     }
 
-    isDeclTy = D->getType() == QT; // If false, then QT may be D's return type
+    IsDeclTy = D->getType() == QT; // If false, then QT may be D's return type
     if (InteropTypeExpr *ITE = D->getInteropTypeExpr()) {
       // External variables can also have itype.
       // Check if the provided declaration is an external
       // variable.
       // For functions, check to see that if we are analyzing
       // function return types.
-      bool AnalyzeITypeExpr = isDeclTy;
+      bool AnalyzeITypeExpr = IsDeclTy;
       if (!AnalyzeITypeExpr) {
         const Type *OrigType = Ty;
         if (isa<FunctionDecl>(D)) {
@@ -343,10 +343,33 @@ PointerVariableConstraint::PointerVariableConstraint(const QualType &QT,
     //    tn fname = ...,
     // where tn is the typedef'ed type name.
     // There is possibly something more elegant to do in the code here.
-    FV = new FVConstraint(Ty, isDeclTy ? D : nullptr,
-                          (IsTypedef ? "" : N), I, C);
-
-  BaseType = tyToStr(Ty);
+    FV = new FVConstraint(Ty, IsDeclTy ? D : nullptr, IsTypedef ? "" : N, I, C);
+
+  // Get a string representing the type without pointer and array indirection.
+  bool FoundMatchingType = false;
+  if (!IsTypedef && D && D->getTypeSourceInfo()) {
+    // Try to extract the type from original source to preserve defines
+    TypeLoc TL = D->getTypeSourceInfo()->getTypeLoc();
+    if (isa<FunctionDecl>(D)) {
+      FoundMatchingType = D->getAsFunction()->getReturnType() == QT;
+      TL = TL.getAs<clang::FunctionTypeLoc>().getReturnLoc();
+    } else {
+      FoundMatchingType = D->getType() == QT;
+    }
+    // Only use this type if the type passed as a parameter to this constructor
+    // agrees with the actual type of the declaration.
+    if (FoundMatchingType) {
+      BaseType = getSourceText(getBaseTypeLoc(TL).getSourceRange(), C);
+
+      // getSourceText returns the empty string when there's a pointer level
+      // inside a macro. Not sure how to handle this, so fall back to tyToStr.
+      if (BaseType.empty())
+        FoundMatchingType = false;
+    }
+  }
+  // Fall back to rebuilding the base type based on type passed to constructor
+  if (!FoundMatchingType)
+    BaseType = tyToStr(Ty);
 
   bool IsWild = !IsGeneric && (isVarArgType(BaseType) || isTypeHasVoid(QT));
   if (IsWild) {

clang/lib/CConv/DeclRewriter.cpp

@@ -103,7 +103,8 @@ void DeclRewriter::rewriteDecls(ASTContext &Context, ProgramInfo &Info,
         // If this function already has a modified signature? and it is not
         // visited by our cast placement visitor then rewrite it.
         std::string NewSig = NewFuncSig[FV->getName()];
-        RewriteThese.insert(new FunctionDeclReplacement(FD, NewSig, true));
+        RewriteThese.insert(new FunctionDeclReplacement(FD, NewSig, true,
+                                                        true));
       }
     }
   }
@@ -217,13 +218,12 @@ void DeclRewriter::rewriteMultiDecl(DeclReplacementTempl<DT, DK> *N,
       // maybe there is still something we can do because Decl refers
       // to a non-macro line.
 
+
       SourceRange Possible(R.getSourceMgr().getExpansionLoc(TR.getBegin()),
-                           D->getLocation());
+                           D->getEndLoc());
 
       if (canRewrite(R, Possible)) {
         R.ReplaceText(Possible, SRewrite);
-        std::string NewStr = " " + D->getName().str();
-        R.InsertTextAfter(D->getLocation(), NewStr);
       } else {
         if (Verbose) {
           errs() << "Still don't know how to re-write VarDecl\n";
@@ -337,9 +337,23 @@ void DeclRewriter::rewriteFunctionDecl(FunctionDeclReplacement *N) {
   //       Additionally, a source range can be (mis) identified as
   //       spanning multiple files. We don't know how to re-write that,
   //       so don't.
-  SourceRange SR = N->getSourceRange(A.getSourceManager());
-  if (canRewrite(R, SR))
+  SourceRange SR = N->getSourceRange();
+  if (canRewrite(R, SR)) {
     R.ReplaceText(SR, N->getReplacement());
+  } else {
+    SourceRange Possible(R.getSourceMgr().getExpansionLoc(SR.getBegin()),
+                         SR.getEnd());
+    if (canRewrite(R, Possible)) {
+      R.ReplaceText(Possible, N->getReplacement());
+    } else if (Verbose) {
+      errs() << "Don't know how to re-write FunctionDecl\n";
+      N->getDecl()->dump();
+      errs() << "at\n";
+      if (N->getStatement())
+        N->getStatement()->dump();
+      errs() << "with " << N->getReplacement() << "\n";
+    }
+  }
 }
 
 bool DeclRewriter::areDeclarationsOnSameLine(DeclReplacement *N1,
@@ -459,21 +473,22 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
   if (!Defnc->hasBody())
     return true;
 
-  // DidAny tracks if we have made any changes to this function declaration.
-  // If no changes are made, then there is no need to rewrite anything, and the
-  // declaration is not added to RewriteThese.
-  bool DidAny = false;
+  // DidAnyParams tracks if we have made any changes to the parameters for this
+  // declarations. If no changes are made, then there is no need to rewrite the
+  // parameter declarations.
+  bool DidAnyParams = false;
 
   // Get rewritten parameter variable declarations
   std::vector<std::string> ParmStrs;
-  for (unsigned i = 0; i < Defnc->numParams(); ++i) {
-    auto *Defn = dyn_cast<PVConstraint>(Defnc->getParamVar(i));
+  for (unsigned I = 0; I < Defnc->numParams(); ++I) {
+    auto *Defn = dyn_cast<PVConstraint>(Defnc->getParamVar(I));
     assert(Defn);
 
+    ParmVarDecl *PVDecl = Definition->getParamDecl(I);
     if (isAValidPVConstraint(Defn) && Defn->anyChanges(CS.getVariables())) {
       // This means Defn has a checked type, so we should rewrite to use this
       // type with an itype if applicable.
-      DidAny = true;
+      DidAnyParams = true;
 
       if (Defn->hasItype() || !Defn->anyArgumentIsWild(CS.getVariables())) {
         // If the definition already has itype or there are no WILD arguments.
@@ -482,7 +497,7 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
         std::string PtypeS =
             Defn->mkString(Info.getConstraints().getVariables());
         PtypeS = PtypeS + getExistingIType(Defn) +
-            ABRewriter.getBoundsString(Defn, Definition->getParamDecl(i));
+            ABRewriter.getBoundsString(Defn, PVDecl);
         ParmStrs.push_back(PtypeS);
       } else {
         // Here, definition is checked type but at least one of the arguments
@@ -492,31 +507,38 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
             Defn->mkString(Info.getConstraints().getVariables(), false, true);
         std::string Bi =
             Defn->getRewritableOriginalTy() + Defn->getName() + " : itype(" +
-                PtypeS + ")" +
-                ABRewriter.getBoundsString(Defn,
-                                       Definition->getParamDecl(i), true);
+                PtypeS + ")" + ABRewriter.getBoundsString(Defn, PVDecl, true);
         ParmStrs.push_back(Bi);
       }
     } else {
-      // If the parameter isn't checked, we can just dump the original
-      // declaration.
-      std::string Scratch = "";
-      raw_string_ostream DeclText(Scratch);
-      Definition->getParamDecl(i)->print(DeclText);
-      ParmStrs.push_back(DeclText.str());
+      // When parameter isn't checked, we just dump the original declaration.
+      ParmStrs.push_back(getSourceText(PVDecl->getSourceRange(), *Context));
     }
   }
 
+
+  if (Defnc->numParams() == 0) {
+    ParmStrs.push_back("void");
+    QualType ReturnTy = FD->getReturnType();
+    QualType Ty = FD->getType();
+    if (!Ty->isFunctionProtoType() && ReturnTy->isPointerType())
+      DidAnyParams = true;
+  }
+
   // Get rewritten return variable
   auto *Defn = dyn_cast<PVConstraint>(Defnc->getReturnVar());
 
   std::string ReturnVar = "";
   std::string ItypeStr = "";
 
+  // Does the same job as DidAnyParams, but with respect to the return value. If
+  // the return does not change, there is no need to rewrite it.
+  bool DidAnyReturn = false;
+
   // Insert a bounds safe interface for the return.
   if (isAValidPVConstraint(Defn) && Defn->anyChanges(CS.getVariables())) {
     // This means we can infer that the return type is a checked type.
-    DidAny = true;
+    DidAnyReturn = true;
     // If the definition has itype or there is no argument which is WILD?
     if (Defn->hasItype() || !Defn->anyArgumentIsWild(CS.getVariables())) {
       // Just get the checked itype
@@ -528,6 +550,10 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
           Defn->mkString(Info.getConstraints().getVariables(), false, true);
       ReturnVar = Defn->getRewritableOriginalTy();
       ItypeStr = " : itype(" + Itype + ")";
+
+      // A small hack here. The inserted itype comes after param declarations,
+      // so we have to rewrite the parameters if we want to insert an itype.
+      DidAnyParams = true;
     }
   } else {
     // This means inside the function, the return value is WILD so the return
@@ -537,37 +563,43 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
     ItypeStr = getExistingIType(Defn);
   }
 
+  // If the return is a function pointer, we need to rewrite the whole
+  // declaration even if no actual changes were made to the parameters. It could
+  // probably be done better, but getting the correct source locations is
+  // painful.
+  if (FD->getReturnType()->isFunctionPointerType() && DidAnyReturn)
+    DidAnyParams = true;
+
   // Combine parameter and return variables rewritings into a single rewriting
   // for the entire function declaration.
-  std::string NewSig =
-      getStorageQualifierString(Definition) + ReturnVar + Defnc->getName()
-          + "(";
-  if (!ParmStrs.empty()) {
+  std::string NewSig = "";
+  if (DidAnyReturn)
+    NewSig = getStorageQualifierString(Definition) + ReturnVar;
+
+  if (DidAnyReturn && DidAnyParams)
+    NewSig += Defnc->getName();
+
+  if (DidAnyParams && !ParmStrs.empty()) {
     // Gather individual parameter strings into a single buffer
     std::ostringstream ConcatParamStr;
     copy(ParmStrs.begin(), ParmStrs.end() - 1,
               std::ostream_iterator<std::string>(ConcatParamStr, ", "));
     ConcatParamStr << ParmStrs.back();
 
-    NewSig = NewSig + ConcatParamStr.str();
+    NewSig += "(" + ConcatParamStr.str();
     // Add varargs.
     if (functionHasVarArgs(Definition))
-      NewSig = NewSig + ", ...";
-    NewSig = NewSig + ")";
-  } else {
-    NewSig = NewSig + "void)";
-    QualType ReturnTy = FD->getReturnType();
-    QualType Ty = FD->getType();
-    if (!Ty->isFunctionProtoType() && ReturnTy->isPointerType())
-      DidAny = true;
+      NewSig += ", ...";
+    NewSig += ")";
   }
   if (!ItypeStr.empty())
     NewSig = NewSig + ItypeStr;
 
   // Add new declarations to RewriteThese if it has changed
-  if (DidAny) {
+  if (DidAnyReturn || DidAnyParams) {
     for (auto *const RD : Definition->redecls())
-      RewriteThese.insert(new FunctionDeclReplacement(RD, NewSig, true));
+      RewriteThese.insert(new FunctionDeclReplacement(RD, NewSig, DidAnyReturn,
+                                                      DidAnyParams));
     // Save the modified function signature.
     if(FD->isStatic()) {
       auto FileName = PersistentSourceLoc::mkPSL(FD, *Context).getFileName();

clang/lib/CConv/RewriteUtils.cpp

@@ -31,7 +31,7 @@ SourceLocation DComp::getDeclBegin(DeclReplacement *D) const {
 }
 
 SourceRange DComp::getReplacementSourceRange(DeclReplacement *D) const {
-  SourceRange Range = D->getSourceRange(SM);
+  SourceRange Range = D->getSourceRange();
 
   // Also take into account whether or not there is a multi-statement
   // decl, because the generated ranges will overlap.

clang/lib/CConv/Utils.cpp

@@ -409,4 +409,11 @@ bool isZeroBoundsExpr(BoundsExpr *BE, const ASTContext &C) {
   // size zero bounds, but checking this would be considerably more complicated
   // and it seems unlikely to show up in real code.
   return false;
+}
+
+TypeLoc getBaseTypeLoc(TypeLoc T) {
+  while (!T.getNextTypeLoc().isNull()
+      && (T.getTypePtr()->isPointerType() || T.getTypePtr()->isArrayType()))
+    T = T.getNextTypeLoc();
+  return T;
 }