Fix incorrect rewritting of function with typdefed type (fix #430) (#436)

Fixes a error in rewriting for function declaration such as

    typedef void foo(int*);
    foo bar;
    void bar(int *a){};

Previously `FunctionDeclBuilder` only replaced the parameter declaration
of `bar` because nothing in the return type needed to be rewritten. This
caused the rewriting of the first declaration to be missing the return
type. With this change, `FunctionDeclBuilder` detects that `bar` is
declared using a typedef type, and generates replacement text for the
entire declaration instead of only the parameter list. This change does not
implement proper constraint generation for these function, so the typedef foo
is not rewritten. 

Reconstruct a parameter declaration if we can't get its original source
due to a macro.

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

Author: john-h-kastner

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, , ) {}