From 6a948c71c102fd436e06f270c40f6a33f41a5b0c Mon Sep 17 00:00:00 2001 From: Joshua Cranmer Date: Mon, 27 May 2013 16:04:18 -0500 Subject: [PATCH] Bug 868285 - Fix static checking builds, part 1: infer MOZ_STACK_CLASS. r=bsmedberg --- build/clang-plugin/clang-plugin.cpp | 120 +++++++++++++++----- build/clang-plugin/tests/TestStackClass.cpp | 7 +- mfbt/Attributes.h | 7 +- 3 files changed, 98 insertions(+), 36 deletions(-) diff --git a/build/clang-plugin/clang-plugin.cpp b/build/clang-plugin/clang-plugin.cpp index 1f4d13e7c7b2..850ecfa7bdb5 100644 --- a/build/clang-plugin/clang-plugin.cpp +++ b/build/clang-plugin/clang-plugin.cpp @@ -11,6 +11,7 @@ #include "clang/Frontend/FrontendPluginRegistry.h" #include "clang/Frontend/MultiplexConsumer.h" #include "clang/Sema/Sema.h" +#include "llvm/ADT/DenseMap.h" #define CLANG_VERSION_FULL (CLANG_VERSION_MAJOR * 100 + CLANG_VERSION_MINOR) @@ -32,6 +33,7 @@ private: class StackClassChecker : public MatchFinder::MatchCallback { public: virtual void run(const MatchFinder::MatchResult &Result); + void noteInferred(QualType T, DiagnosticsEngine &Diag); }; StackClassChecker stackClassChecker; @@ -87,16 +89,6 @@ public: if (hasCustomAnnotation(*M, "moz_must_override")) must_overrides.push_back(*M); } - - // While we are looping over parent classes, we'll also check to make sure - // that the subclass has the annotation if the superclass does. - if (hasCustomAnnotation(parent, "moz_stack_class") && - !hasCustomAnnotation(d, "moz_stack_class")) { - unsigned badInheritID = Diag.getDiagnosticIDs()->getCustomDiagID( - DiagnosticIDs::Error, "%0 inherits from a stack class %1"); - Diag.Report(d->getLocation(), badInheritID) - << d->getDeclName() << parent->getDeclName(); - } } for (OverridesVector::iterator it = must_overrides.begin(); @@ -123,6 +115,60 @@ public: return true; } }; + +DenseMap stackClassCauses; + +bool isStackClass(QualType T); + +bool isStackClass(CXXRecordDecl *D) { + // Normalize so that D points to the definition if it exists. If it doesn't, + // then we can't allocate it anyways. + if (!D->hasDefinition()) + return false; + D = D->getDefinition(); + // Base class: anyone with this annotation is obviously a stack class + if (MozChecker::hasCustomAnnotation(D, "moz_stack_class")) + return true; + + // See if we cached the result. + DenseMap::iterator it = + stackClassCauses.find(D); + if (it != stackClassCauses.end()) { + // If the cause is NULL, then this is not a stack class. + return it->second != NULL; + } + + // Look through all base cases to figure out if the parent is a stack class. + for (CXXRecordDecl::base_class_iterator base = D->bases_begin(), + e = D->bases_end(); base != e; ++base) { + if (isStackClass(base->getType())) { + stackClassCauses.insert(std::make_pair(D, + base->getType()->getAsCXXRecordDecl())); + return true; + } + } + + // Maybe it has a member which is a stack class. + for (RecordDecl::field_iterator field = D->field_begin(), e = D->field_end(); + field != e; ++field) { + if (isStackClass(field->getType())) { + stackClassCauses.insert(std::make_pair(D, *field)); + return true; + } + } + + // Nope, this class is not a stack class. + stackClassCauses.insert(std::make_pair(D, (const Decl*)0)); + return false; +} + +bool isStackClass(QualType T) { + while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe()) + T = arrTy->getElementType(); + CXXRecordDecl *clazz = T->getAsCXXRecordDecl(); + return clazz && isStackClass(clazz); +} + } namespace clang { @@ -131,11 +177,7 @@ namespace ast_matchers { /// This matcher will match any class with the stack class assertion or an /// array of such classes. AST_MATCHER(QualType, stackClassAggregate) { - QualType t = Node; - while (const ArrayType *arrTy = t->getAsArrayTypeUnsafe()) - t = arrTy->getElementType(); - CXXRecordDecl *clazz = t->getAsCXXRecordDecl(); - return clazz && MozChecker::hasCustomAnnotation(clazz, "moz_stack_class"); + return isStackClass(Node); } } } @@ -151,10 +193,6 @@ DiagnosticsMatcher::DiagnosticsMatcher() { astMatcher.addMatcher(newExpr(hasType(pointerType( pointee(stackClassAggregate()) ))).bind("node"), &stackClassChecker); - // Stack class assertion: a stack-class field is permitted only if it's a - // member of a class with the annotation - astMatcher.addMatcher(fieldDecl(hasType(stackClassAggregate())).bind("field"), - &stackClassChecker); } void DiagnosticsMatcher::StackClassChecker::run( @@ -166,28 +204,48 @@ void DiagnosticsMatcher::StackClassChecker::run( // Ignore the match if it's a local variable. if (d->hasLocalStorage()) return; + Diag.Report(d->getLocation(), stackID) << d->getType(); + noteInferred(d->getType(), Diag); } else if (const CXXNewExpr *expr = Result.Nodes.getNodeAs("node")) { // If it's placement new, then this match doesn't count. if (expr->getNumPlacementArgs() > 0) return; Diag.Report(expr->getStartLoc(), stackID) << expr->getAllocatedType(); - } else if (const FieldDecl *field = - Result.Nodes.getNodeAs("field")) { - // AST matchers don't let me get the class that contains a field... - const RecordDecl *parent = field->getParent(); - if (!MozChecker::hasCustomAnnotation(parent, "moz_stack_class")) { - // We use a more verbose error message here. - unsigned stackID = Diag.getDiagnosticIDs()->getCustomDiagID( - DiagnosticIDs::Error, - "member of type %0 in class %1 that is not a stack class"); - Diag.Report(field->getLocation(), stackID) << field->getType() << - parent->getDeclName(); - } + noteInferred(expr->getAllocatedType(), Diag); } } +void DiagnosticsMatcher::StackClassChecker::noteInferred(QualType T, + DiagnosticsEngine &Diag) { + unsigned inheritsID = Diag.getDiagnosticIDs()->getCustomDiagID( + DiagnosticIDs::Note, + "%0 is a stack class because it inherits from a stack class %1"); + unsigned memberID = Diag.getDiagnosticIDs()->getCustomDiagID( + DiagnosticIDs::Note, + "%0 is a stack class because member %1 is a stack class %2"); + + // Find the CXXRecordDecl that is the stack class of interest + while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe()) + T = arrTy->getElementType(); + CXXRecordDecl *clazz = T->getAsCXXRecordDecl(); + + // Direct result, we're done. + if (MozChecker::hasCustomAnnotation(clazz, "moz_stack_class")) + return; + + const Decl *cause = stackClassCauses[clazz]; + if (const CXXRecordDecl *CRD = dyn_cast(cause)) { + Diag.Report(clazz->getLocation(), inheritsID) << T << CRD->getDeclName(); + } else if (const FieldDecl *FD = dyn_cast(cause)) { + Diag.Report(FD->getLocation(), memberID) << T << FD << FD->getType(); + } + + // Recursively follow this back. + noteInferred(cast(cause)->getType(), Diag); +} + class MozCheckAction : public PluginASTAction { public: ASTConsumer *CreateASTConsumer(CompilerInstance &CI, StringRef fileName) { diff --git a/build/clang-plugin/tests/TestStackClass.cpp b/build/clang-plugin/tests/TestStackClass.cpp index 4ad6398fe56e..1cce9f4abca6 100644 --- a/build/clang-plugin/tests/TestStackClass.cpp +++ b/build/clang-plugin/tests/TestStackClass.cpp @@ -35,7 +35,7 @@ void misuseStackClass(int len) { Stack notValid; // expected-error {{variable of type 'Stack' only valid on the stack}} struct RandomClass { - Stack nonstaticMember; // expected-error {{member of type 'Stack' in class 'RandomClass' that is not a stack class}} + Stack nonstaticMember; // expected-note {{'RandomClass' is a stack class because member 'nonstaticMember' is a stack class 'Stack'}} static Stack staticMember; // expected-error {{variable of type 'Stack' only valid on the stack}} }; struct MOZ_STACK_CLASS RandomStackClass { @@ -43,5 +43,8 @@ struct MOZ_STACK_CLASS RandomStackClass { static Stack staticMember; // expected-error {{variable of type 'Stack' only valid on the stack}} }; -struct BadInherit : Stack {}; // expected-error {{'BadInherit' inherits from a stack class 'Stack'}} +struct BadInherit : Stack {}; // expected-note {{'BadInherit' is a stack class because it inherits from a stack class 'Stack'}} struct MOZ_STACK_CLASS GoodInherit : Stack {}; + +BadInherit moreInvalid; // expected-error {{variable of type 'BadInherit' only valid on the stack}} +RandomClass evenMoreInvalid; // expected-error {{variable of type 'RandomClass' only valid on the stack}} diff --git a/mfbt/Attributes.h b/mfbt/Attributes.h index 89f3641fc9f8..b8f7b610c9b3 100644 --- a/mfbt/Attributes.h +++ b/mfbt/Attributes.h @@ -382,9 +382,10 @@ * MOZ_STACK_CLASS: Applies to all classes. Any class with this annotation is * expected to live on the stack, so it is a compile-time error to use it, or * an array of such objects, as a global or static variable, or as the type of - * a new expression (unless placement new is being used). It may be a base or - * a member of another class only if both classes are marked with this - * annotation. + * a new expression (unless placement new is being used). If a member of + * another class uses this class, or if another class inherits from this + * class, then it is considered to be a stack class as well, although this + * attribute need not be provided in such cases. */ #ifdef MOZ_CLANG_PLUGIN # define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))