Fix incorrect rewritting of function with typdefed type

clang/include/clang/3C/DeclRewriter.h

@@ -122,6 +122,8 @@ class FunctionDeclBuilder : public RecursiveASTVisitor<FunctionDeclBuilder> {
   void buildItypeDecl(PVConstraint *Defn, DeclaratorDecl *Decl,
                       std::string &Type, std::string &IType, bool &RewriteParm,
                       bool &RewriteRet);
+
+  bool hasDeclWithTypedef(const FunctionDecl *FD);
 };
 
 class FieldFinder : public RecursiveASTVisitor<FieldFinder> {

clang/include/clang/3C/Utils.h

@@ -142,7 +142,12 @@ bool isVarArgType(const std::string &TypeName);
 bool isStructOrUnionType(clang::VarDecl *VD);
 
 // Helper method to print a Type in a way that can be represented in the source.
-std::string tyToStr(const clang::Type *T);
+// If Name is given, it is included as the variable name (which otherwise isn't
+// trivial to do with function pointers, etc.).
+std::string tyToStr(const clang::Type *T, const std::string &Name = "");
+
+// Same as tyToStr with a QualType.
+std::string qtyToStr(clang::QualType QT, const std::string &Name = "");
 
 // Get the end source location of the end of the provided function.
 clang::SourceLocation getFunctionDeclRParen(clang::FunctionDecl *FD,

clang/lib/3C/DeclRewriter.cpp

@@ -539,13 +539,13 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
   if (!Defnc->hasBody())
     return true;
 
-  // 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. This will also be set to true if an itype is added
-  // to the return, since return itypes are inserted afters params.
+  // RewriteParams and RewriteReturn track if we will need to rewrite the
+  // parameter and return type declarations on this function. They are first
+  // set to true if any changes are made to the types of the parameter and
+  // return. If a type has changed, then it must be rewritten. There are then
+  // some special circumstances which require rewriting the parameter or return
+  // even when the type as not changed.
   bool RewriteParams = false;
-  // Does the same job as RewriteParams, but with respect to the return value.
-  // If the return does not change, there is no need to rewrite it.
   bool RewriteReturn = false;
 
   // Get rewritten parameter variable declarations
@@ -574,12 +574,27 @@ bool FunctionDeclBuilder::VisitFunctionDecl(FunctionDecl *FD) {
                      ReturnVar, ItypeStr, RewriteParams, RewriteReturn);
 
   // 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.
+  // declaration even if no actual changes were made to the parameters because
+  // the parameter for the function pointer type appear later in the source than
+  // the parameters for the function declaration. It could probably be done
+  // better, but getting the correct source locations is painful.
   if (FD->getReturnType()->isFunctionPointerType() && RewriteReturn)
     RewriteParams = true;
 
+  // If the function is declared using a typedef for the function type, then we
+  // need to rewrite parameters and the return if either would have been
+  // rewritten. What this does is expand the typedef to the full function type
+  // to avoid the problem of rewriting inside the typedef.
+  // FIXME: If issue #437 is fixed in way that preserves typedefs on function
+  //        declarations, then this conditional should be removed to enable
+  //        separate rewriting of return type and parameters on the
+  //        corresponding definition.
+  //        https://github.com/correctcomputation/checkedc-clang/issues/437
+  if ((RewriteReturn || RewriteParams) && hasDeclWithTypedef(FD)) {
+    RewriteParams = true;
+    RewriteReturn = true;
+  }
+
   // Combine parameter and return variables rewritings into a single rewriting
   // for the entire function declaration.
   std::string NewSig = "";
@@ -647,6 +662,10 @@ void FunctionDeclBuilder::buildItypeDecl(PVConstraint *Defn,
   return;
 }
 
+// Note: For a parameter, Type + IType will give the full declaration (including
+// the name) but the breakdown between Type and IType is not guaranteed. For a
+// return, Type will be what goes before the name and IType will be what goes
+// after the parentheses.
 void
 FunctionDeclBuilder::buildDeclVar(PVConstraint *IntCV, PVConstraint *ExtCV,
                                   DeclaratorDecl *Decl, std::string &Type,
@@ -668,16 +687,34 @@ FunctionDeclBuilder::buildDeclVar(PVConstraint *IntCV, PVConstraint *ExtCV,
       buildItypeDecl(ExtCV, Decl, Type, IType, RewriteParm, RewriteRet);
     return;
   }
-  // Variables that do not need to be rewritten fall through to here. Type
-  // strings are taken unchanged from the original source.
-  if (isa<ParmVarDecl>(Decl)) {
-    Type = getSourceText(Decl->getSourceRange(), *Context);
-    IType = "";
+  // Variables that do not need to be rewritten fall through to here.
+  // For parameter variables, we try to extract the declaration from the source
+  // code. This preserves macros and other formatting. This isn't possible for
+  // return variables because the itype on returns is located after the
+  // parameter list. Sometimes we cannot get the original source for a parameter
+  // declaration, for example if a function prototype is declared using a
+  // typedef or the parameter declaration is inside a macro. For these cases, we
+  // just fall back to reconstructing the declaration from the PVConstraint.
+  ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(Decl);
+  if (PVD) {
+    SourceRange Range = PVD->getSourceRange();
+    if (Range.isValid()) {
+      Type = getSourceText(Range, *Context);
+      if (!Type.empty()) {
+        // Great, we got the original source including any itype and bounds.
+        IType = "";
+        return;
+      }
+    }
+    // Otherwise, reconstruct the name and type, and reuse the code below for
+    // the itype and bounds.
+    // TODO: Do we care about `register` or anything else this doesn't handle?
+    Type = qtyToStr(PVD->getOriginalType(), PVD->getNameAsString());
   } else {
     Type = ExtCV->getOriginalTy() + " ";
-    IType = getExistingIType(ExtCV);
-    IType += ABRewriter.getBoundsString(ExtCV, Decl, !IType.empty());
   }
+  IType = getExistingIType(ExtCV);
+  IType += ABRewriter.getBoundsString(ExtCV, Decl, !IType.empty());
 }
 
 std::string FunctionDeclBuilder::getExistingIType(ConstraintVariable *DeclC) {
@@ -692,6 +729,37 @@ bool FunctionDeclBuilder::isFunctionVisited(std::string FuncName) {
   return VisitedSet.find(FuncName) != VisitedSet.end();
 }
 
+// Given a function declaration figure out if this declaration or any other
+// declaration of the same function is declared using a typedefed function type.
+bool FunctionDeclBuilder::hasDeclWithTypedef(const FunctionDecl *FD) {
+  for (FunctionDecl *FDIter : FD->redecls()) {
+    // If the declaration type is TypedefType, then this is definitely declared
+    // using a typedef. This only happens when the typedefed declaration is the
+    // first declaration of a function.
+    if (isa_and_nonnull<TypedefType>(FDIter->getType().getTypePtrOrNull()))
+      return true;
+    // Next look for a TypeDefTypeLoc. This is present on the typedefed
+    // declaration even when it is not the first declaration.
+    TypeSourceInfo *TSI = FDIter->getTypeSourceInfo();
+    if (TSI) {
+      if (!TSI->getTypeLoc().getAs<TypedefTypeLoc>().isNull())
+        return true;
+    } else {
+      // This still could possibly be a typedef type if TSI was NULL.
+      // TypeSourceInfo is null for implicit function declarations, so if a
+      // implicit declaration uses a typedef, it will be missed. That's fine
+      // since an implicit declaration can't be rewritten anyways.
+      // There might be other ways it can be null that I'm not aware of.
+      if (Verbose) {
+        llvm::errs() << "Unable to conclusively determine if a function "
+                     << "declaration uses a typedef.\n";
+        FDIter->dump();
+      }
+    }
+  }
+  return false;
+}
+
 bool FieldFinder::VisitFieldDecl(FieldDecl *FD) {
   GVG.addGlobalDecl(FD);
   return true;

clang/lib/3C/Utils.cpp

@@ -224,15 +224,14 @@ bool isStructOrUnionType(clang::VarDecl *VD) {
          VD->getType().getTypePtr()->isUnionType();
 }
 
-std::string tyToStr(const clang::Type *T) {
-  if (auto TDT = dyn_cast<TypedefType>(T)) {
-    auto D = TDT->getDecl();
-    std::string s = D->getNameAsString();
-    return s;
-  } else {
-    QualType QT(T, 0);
-    return QT.getAsString();
-  }
+std::string qtyToStr(clang::QualType QT, const std::string &Name) {
+  std::string S = Name;
+  QT.getAsStringInternal(S, LangOptions());
+  return S;
+}
+
+std::string tyToStr(const clang::Type *T, const std::string &Name) {
+  return qtyToStr(QualType(T, 0), Name);
 }
 
 Expr *removeAuxillaryCasts(Expr *E) {

clang/test/3C/function_typedef.c

@@ -0,0 +1,37 @@
+// RUN: rm -rf %t*
+// RUN: 3c -base-dir=%S -alltypes -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s
+// RUN: 3c -base-dir=%S -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_NOALL","CHECK" %s
+// RUN: 3c -base-dir=%S -addcr %s -- | %clang -c -fcheckedc-extension -x c -o /dev/null -
+// RUN: 3c -base-dir=%S -output-dir=%t.checked -alltypes %s --
+// RUN: 3c -base-dir=%t.checked -alltypes %t.checked/function_typedef.c -- | diff %t.checked/function_typedef.c -
+
+// Tests for the single file case in issue #430
+// Functions declared using a typedef should be rewritten in a way that doesn't
+// crash 3C or generate uncompilable code.  The expected output for these tests
+// is expected to change when issue #437 is resolved.
+
+typedef void foo(int*);
+foo foo_impl;
+void foo_imp(int *a) {};
+//CHECK: foo foo_impl;
+//CHECK: void foo_imp(_Ptr<int> a) _Checked {};
+
+typedef int *bar();
+bar bar_impl;
+int *bar_impl() {
+  return 0;
+};
+//CHECK: _Ptr<int> bar_impl(void);
+//CHECK: _Ptr<int> bar_impl(void) _Checked {
+//CHECK:   return 0;
+//CHECK: };
+
+typedef int *baz(int *);
+baz baz_impl;
+int *baz_impl(int *a) {
+  return 0;
+};
+//CHECK: _Ptr<int> baz_impl(_Ptr<int> a);
+//CHECK: _Ptr<int> baz_impl(_Ptr<int> a) _Checked {
+//CHECK:   return 0;
+//CHECK: };

clang/test/3C/function_typedef_multi.c

@@ -0,0 +1,13 @@
+// RUN: rm -rf %t*
+// RUN: 3c -base-dir=%S -addcr -output-dir=%t.checked %S/function_typedef_multi.c %S/function_typedef_multi.h --
+// RUN: test ! -f %t.checked/function_typedef_multi.h -a ! -f %t.checked/function_typedef_multi.c
+
+// Test for the two file case in issue #430
+// This test caused an assertion to fail prior to PR #436
+// This test uses function_typedef_multi.h. The header is deliberately not
+// included in this file. Including it prevented the assertion fail even
+// without the changes in PR #436.
+
+int foo(int a, int b[1]) {
+  return 0;
+}

clang/test/3C/function_typedef_multi.h

@@ -0,0 +1,3 @@
+// Used with function_typedef_multi.c
+typedef int a(int, int[1]);
+a foo;

clang/test/3C/params_in_macro.c

@@ -0,0 +1,24 @@
+// Test that if FunctionDeclBuilder::buildDeclVar cannot get the original source
+// for a parameter declaration due to a macro, it reconstructs the declaration
+// (including the name) instead of leaving a blank.
+
+// RUN: 3c -base-dir=%S %s -- | FileCheck -match-full-lines %s
+// RUN: 3c -base-dir=%S %s -- | %clang -c -fcheckedc-extension -x c -o /dev/null -
+
+// 3C is not idempotent on this file because the first pass constrains the
+// pointers in the macros to wild but then inlines the macros, so the second
+// pass will make those pointers checked. So don't try to test idempotence.
+
+typedef double mydouble;
+
+// TODO: FunctionDeclBuilder::buildDeclVar should be able to handle an itype
+// here, but currently the PointerConstraintVariable constructor asserts when it
+// fails to retrieve the original source of the itype declaration.
+#define parms1 volatile mydouble d, void (*f)(void)
+#define parms2 int *const y : count(7), _Ptr<char> z
+
+void test(parms1, int *x, parms2) {}
+// CHECK: void test(volatile mydouble d, void (*f)(void), _Ptr<int> x, int *const y : count(7), _Ptr<char> z) {}
+
+// Before the bug fix, we got:
+// void test(, , _Ptr<int> x, , ) {}