diff --git a/clang/include/clang/3C/DeclRewriter.h b/clang/include/clang/3C/DeclRewriter.h index 64a9471af25f..06d72a4d645d 100644 --- a/clang/include/clang/3C/DeclRewriter.h +++ b/clang/include/clang/3C/DeclRewriter.h @@ -122,6 +122,8 @@ class FunctionDeclBuilder : public RecursiveASTVisitor { 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 { diff --git a/clang/include/clang/3C/Utils.h b/clang/include/clang/3C/Utils.h index 52558cf89416..cf93d2004b62 100644 --- a/clang/include/clang/3C/Utils.h +++ b/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, diff --git a/clang/lib/3C/DeclRewriter.cpp b/clang/lib/3C/DeclRewriter.cpp index 90e63dff5f9d..d2f1dd7898d1 100644 --- a/clang/lib/3C/DeclRewriter.cpp +++ b/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(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(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(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().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; diff --git a/clang/lib/3C/Utils.cpp b/clang/lib/3C/Utils.cpp index 250adaaeb329..e4f1422a13e0 100644 --- a/clang/lib/3C/Utils.cpp +++ b/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(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) { diff --git a/clang/test/3C/function_typedef.c b/clang/test/3C/function_typedef.c new file mode 100644 index 000000000000..2a09cce2387c --- /dev/null +++ b/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 a) _Checked {}; + +typedef int *bar(); +bar bar_impl; +int *bar_impl() { + return 0; +}; +//CHECK: _Ptr bar_impl(void); +//CHECK: _Ptr bar_impl(void) _Checked { +//CHECK: return 0; +//CHECK: }; + +typedef int *baz(int *); +baz baz_impl; +int *baz_impl(int *a) { + return 0; +}; +//CHECK: _Ptr baz_impl(_Ptr a); +//CHECK: _Ptr baz_impl(_Ptr a) _Checked { +//CHECK: return 0; +//CHECK: }; diff --git a/clang/test/3C/function_typedef_multi.c b/clang/test/3C/function_typedef_multi.c new file mode 100644 index 000000000000..cd12c5957f94 --- /dev/null +++ b/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; +} diff --git a/clang/test/3C/function_typedef_multi.h b/clang/test/3C/function_typedef_multi.h new file mode 100644 index 000000000000..ffbbc8281c7f --- /dev/null +++ b/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; diff --git a/clang/test/3C/params_in_macro.c b/clang/test/3C/params_in_macro.c new file mode 100644 index 000000000000..1850419d6fff --- /dev/null +++ b/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 z + +void test(parms1, int *x, parms2) {} +// CHECK: void test(volatile mydouble d, void (*f)(void), _Ptr x, int *const y : count(7), _Ptr z) {} + +// Before the bug fix, we got: +// void test(, , _Ptr x, , ) {}