| OLD | NEW |
|---|
| (Empty) |
| 1 // Copyright (c) 2012 The Chromium Authors. All rights reserved. | |
| 2 // Use of this source code is governed by a BSD-style license that can be | |
| 3 // found in the LICENSE file. | |
| 4 | |
| 5 // This file defines a bunch of recurring problems in the Chromium C++ code. | |
| 6 // | |
| 7 // Checks that are implemented: | |
| 8 // - Constructors/Destructors should not be inlined if they are of a complex | |
| 9 // class type. | |
| 10 // - Missing "virtual" keywords on methods that should be virtual. | |
| 11 // - Non-annotated overriding virtual methods. | |
| 12 // - Virtual methods with nonempty implementations in their headers. | |
| 13 // - Classes that derive from base::RefCounted / base::RefCountedThreadSafe | |
| 14 // should have protected or private destructors. | |
| 15 // - WeakPtrFactory members that refer to their outer class should be the last | |
| 16 // member. | |
| 17 // - Enum types with a xxxx_LAST or xxxxLast const actually have that constant | |
| 18 // have the maximal value for that type. | |
| 19 | |
| 20 #include "clang/AST/ASTConsumer.h" | |
| 21 #include "clang/AST/AST.h" | |
| 22 #include "clang/AST/Attr.h" | |
| 23 #include "clang/AST/CXXInheritance.h" | |
| 24 #include "clang/AST/TypeLoc.h" | |
| 25 #include "clang/Basic/SourceManager.h" | |
| 26 #include "clang/Frontend/CompilerInstance.h" | |
| 27 #include "clang/Frontend/FrontendPluginRegistry.h" | |
| 28 #include "clang/Lex/Lexer.h" | |
| 29 #include "llvm/Support/raw_ostream.h" | |
| 30 | |
| 31 #include "ChromeClassTester.h" | |
| 32 | |
| 33 using namespace clang; | |
| 34 | |
| 35 namespace { | |
| 36 | |
| 37 const char kMethodRequiresOverride[] = | |
| 38 "[chromium-style] Overriding method must be marked with OVERRIDE."; | |
| 39 const char kMethodRequiresVirtual[] = | |
| 40 "[chromium-style] Overriding method must have \"virtual\" keyword."; | |
| 41 const char kNoExplicitDtor[] = | |
| 42 "[chromium-style] Classes that are ref-counted should have explicit " | |
| 43 "destructors that are declared protected or private."; | |
| 44 const char kPublicDtor[] = | |
| 45 "[chromium-style] Classes that are ref-counted should have " | |
| 46 "destructors that are declared protected or private."; | |
| 47 const char kProtectedNonVirtualDtor[] = | |
| 48 "[chromium-style] Classes that are ref-counted and have non-private " | |
| 49 "destructors should declare their destructor virtual."; | |
| 50 const char kWeakPtrFactoryOrder[] = | |
| 51 "[chromium-style] WeakPtrFactory members which refer to their outer class " | |
| 52 "must be the last member in the outer class definition."; | |
| 53 const char kBadLastEnumValue[] = | |
| 54 "[chromium-style] _LAST/Last constants of enum types must have the maximal " | |
| 55 "value for any constant of that type."; | |
| 56 const char kNoteInheritance[] = | |
| 57 "[chromium-style] %0 inherits from %1 here"; | |
| 58 const char kNoteImplicitDtor[] = | |
| 59 "[chromium-style] No explicit destructor for %0 defined"; | |
| 60 const char kNotePublicDtor[] = | |
| 61 "[chromium-style] Public destructor declared here"; | |
| 62 const char kNoteProtectedNonVirtualDtor[] = | |
| 63 "[chromium-style] Protected non-virtual destructor declared here"; | |
| 64 | |
| 65 bool TypeHasNonTrivialDtor(const Type* type) { | |
| 66 if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl()) | |
| 67 return !cxx_r->hasTrivialDestructor(); | |
| 68 | |
| 69 return false; | |
| 70 } | |
| 71 | |
| 72 // Returns the underlying Type for |type| by expanding typedefs and removing | |
| 73 // any namespace qualifiers. This is similar to desugaring, except that for | |
| 74 // ElaboratedTypes, desugar will unwrap too much. | |
| 75 const Type* UnwrapType(const Type* type) { | |
| 76 if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type)) | |
| 77 return UnwrapType(elaborated->getNamedType().getTypePtr()); | |
| 78 if (const TypedefType* typedefed = dyn_cast<TypedefType>(type)) | |
| 79 return UnwrapType(typedefed->desugar().getTypePtr()); | |
| 80 return type; | |
| 81 } | |
| 82 | |
| 83 struct FindBadConstructsOptions { | |
| 84 FindBadConstructsOptions() : check_base_classes(false), | |
| 85 check_virtuals_in_implementations(true), | |
| 86 check_weak_ptr_factory_order(false), | |
| 87 check_enum_last_value(false) { | |
| 88 } | |
| 89 bool check_base_classes; | |
| 90 bool check_virtuals_in_implementations; | |
| 91 bool check_weak_ptr_factory_order; | |
| 92 bool check_enum_last_value; | |
| 93 }; | |
| 94 | |
| 95 // Searches for constructs that we know we don't want in the Chromium code base. | |
| 96 class FindBadConstructsConsumer : public ChromeClassTester { | |
| 97 public: | |
| 98 FindBadConstructsConsumer(CompilerInstance& instance, | |
| 99 const FindBadConstructsOptions& options) | |
| 100 : ChromeClassTester(instance), | |
| 101 options_(options) { | |
| 102 // Register warning/error messages. | |
| 103 diag_method_requires_override_ = diagnostic().getCustomDiagID( | |
| 104 getErrorLevel(), kMethodRequiresOverride); | |
| 105 diag_method_requires_virtual_ = diagnostic().getCustomDiagID( | |
| 106 getErrorLevel(), kMethodRequiresVirtual); | |
| 107 diag_no_explicit_dtor_ = diagnostic().getCustomDiagID( | |
| 108 getErrorLevel(), kNoExplicitDtor); | |
| 109 diag_public_dtor_ = diagnostic().getCustomDiagID( | |
| 110 getErrorLevel(), kPublicDtor); | |
| 111 diag_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID( | |
| 112 getErrorLevel(), kProtectedNonVirtualDtor); | |
| 113 diag_weak_ptr_factory_order_ = diagnostic().getCustomDiagID( | |
| 114 getErrorLevel(), kWeakPtrFactoryOrder); | |
| 115 diag_bad_enum_last_value_ = diagnostic().getCustomDiagID( | |
| 116 getErrorLevel(), kBadLastEnumValue); | |
| 117 | |
| 118 // Registers notes to make it easier to interpret warnings. | |
| 119 diag_note_inheritance_ = diagnostic().getCustomDiagID( | |
| 120 DiagnosticsEngine::Note, kNoteInheritance); | |
| 121 diag_note_implicit_dtor_ = diagnostic().getCustomDiagID( | |
| 122 DiagnosticsEngine::Note, kNoteImplicitDtor); | |
| 123 diag_note_public_dtor_ = diagnostic().getCustomDiagID( | |
| 124 DiagnosticsEngine::Note, kNotePublicDtor); | |
| 125 diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID( | |
| 126 DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor); | |
| 127 } | |
| 128 | |
| 129 virtual void CheckChromeClass(SourceLocation record_location, | |
| 130 CXXRecordDecl* record) { | |
| 131 bool implementation_file = InImplementationFile(record_location); | |
| 132 | |
| 133 if (!implementation_file) { | |
| 134 // Only check for "heavy" constructors/destructors in header files; | |
| 135 // within implementation files, there is no performance cost. | |
| 136 CheckCtorDtorWeight(record_location, record); | |
| 137 } | |
| 138 | |
| 139 if (!implementation_file || options_.check_virtuals_in_implementations) { | |
| 140 bool warn_on_inline_bodies = !implementation_file; | |
| 141 | |
| 142 // Check that all virtual methods are marked accordingly with both | |
| 143 // virtual and OVERRIDE. | |
| 144 CheckVirtualMethods(record_location, record, warn_on_inline_bodies); | |
| 145 } | |
| 146 | |
| 147 CheckRefCountedDtors(record_location, record); | |
| 148 | |
| 149 if (options_.check_weak_ptr_factory_order) | |
| 150 CheckWeakPtrFactoryMembers(record_location, record); | |
| 151 } | |
| 152 | |
| 153 virtual void CheckChromeEnum(SourceLocation enum_location, | |
| 154 EnumDecl* enum_decl) { | |
| 155 if (!options_.check_enum_last_value) | |
| 156 return; | |
| 157 | |
| 158 bool got_one = false; | |
| 159 bool is_signed = false; | |
| 160 llvm::APSInt max_so_far; | |
| 161 EnumDecl::enumerator_iterator iter; | |
| 162 for (iter = enum_decl->enumerator_begin(); | |
| 163 iter != enum_decl->enumerator_end(); ++iter) { | |
| 164 llvm::APSInt current_value = iter->getInitVal(); | |
| 165 if (!got_one) { | |
| 166 max_so_far = current_value; | |
| 167 is_signed = current_value.isSigned(); | |
| 168 got_one = true; | |
| 169 } else { | |
| 170 if (is_signed != current_value.isSigned()) { | |
| 171 // This only happens in some cases when compiling C (not C++) files, | |
| 172 // so it is OK to bail out here. | |
| 173 return; | |
| 174 } | |
| 175 if (current_value > max_so_far) | |
| 176 max_so_far = current_value; | |
| 177 } | |
| 178 } | |
| 179 for (iter = enum_decl->enumerator_begin(); | |
| 180 iter != enum_decl->enumerator_end(); ++iter) { | |
| 181 std::string name = iter->getNameAsString(); | |
| 182 if (((name.size() > 4 && | |
| 183 name.compare(name.size() - 4, 4, "Last") == 0) || | |
| 184 (name.size() > 5 && | |
| 185 name.compare(name.size() - 5, 5, "_LAST") == 0)) && | |
| 186 iter->getInitVal() < max_so_far) { | |
| 187 diagnostic().Report(iter->getLocation(), diag_bad_enum_last_value_); | |
| 188 } | |
| 189 } | |
| 190 } | |
| 191 | |
| 192 private: | |
| 193 // The type of problematic ref-counting pattern that was encountered. | |
| 194 enum RefcountIssue { | |
| 195 None, | |
| 196 ImplicitDestructor, | |
| 197 PublicDestructor | |
| 198 }; | |
| 199 | |
| 200 FindBadConstructsOptions options_; | |
| 201 | |
| 202 unsigned diag_method_requires_override_; | |
| 203 unsigned diag_method_requires_virtual_; | |
| 204 unsigned diag_no_explicit_dtor_; | |
| 205 unsigned diag_public_dtor_; | |
| 206 unsigned diag_protected_non_virtual_dtor_; | |
| 207 unsigned diag_weak_ptr_factory_order_; | |
| 208 unsigned diag_bad_enum_last_value_; | |
| 209 unsigned diag_note_inheritance_; | |
| 210 unsigned diag_note_implicit_dtor_; | |
| 211 unsigned diag_note_public_dtor_; | |
| 212 unsigned diag_note_protected_non_virtual_dtor_; | |
| 213 | |
| 214 // Prints errors if the constructor/destructor weight is too heavy. | |
| 215 void CheckCtorDtorWeight(SourceLocation record_location, | |
| 216 CXXRecordDecl* record) { | |
| 217 // We don't handle anonymous structs. If this record doesn't have a | |
| 218 // name, it's of the form: | |
| 219 // | |
| 220 // struct { | |
| 221 // ... | |
| 222 // } name_; | |
| 223 if (record->getIdentifier() == NULL) | |
| 224 return; | |
| 225 | |
| 226 // Count the number of templated base classes as a feature of whether the | |
| 227 // destructor can be inlined. | |
| 228 int templated_base_classes = 0; | |
| 229 for (CXXRecordDecl::base_class_const_iterator it = record->bases_begin(); | |
| 230 it != record->bases_end(); ++it) { | |
| 231 if (it->getTypeSourceInfo()->getTypeLoc().getTypeLocClass() == | |
| 232 TypeLoc::TemplateSpecialization) { | |
| 233 ++templated_base_classes; | |
| 234 } | |
| 235 } | |
| 236 | |
| 237 // Count the number of trivial and non-trivial member variables. | |
| 238 int trivial_member = 0; | |
| 239 int non_trivial_member = 0; | |
| 240 int templated_non_trivial_member = 0; | |
| 241 for (RecordDecl::field_iterator it = record->field_begin(); | |
| 242 it != record->field_end(); ++it) { | |
| 243 CountType(it->getType().getTypePtr(), | |
| 244 &trivial_member, | |
| 245 &non_trivial_member, | |
| 246 &templated_non_trivial_member); | |
| 247 } | |
| 248 | |
| 249 // Check to see if we need to ban inlined/synthesized constructors. Note | |
| 250 // that the cutoffs here are kind of arbitrary. Scores over 10 break. | |
| 251 int dtor_score = 0; | |
| 252 // Deriving from a templated base class shouldn't be enough to trigger | |
| 253 // the ctor warning, but if you do *anything* else, it should. | |
| 254 // | |
| 255 // TODO(erg): This is motivated by templated base classes that don't have | |
| 256 // any data members. Somehow detect when templated base classes have data | |
| 257 // members and treat them differently. | |
| 258 dtor_score += templated_base_classes * 9; | |
| 259 // Instantiating a template is an insta-hit. | |
| 260 dtor_score += templated_non_trivial_member * 10; | |
| 261 // The fourth normal class member should trigger the warning. | |
| 262 dtor_score += non_trivial_member * 3; | |
| 263 | |
| 264 int ctor_score = dtor_score; | |
| 265 // You should be able to have 9 ints before we warn you. | |
| 266 ctor_score += trivial_member; | |
| 267 | |
| 268 if (ctor_score >= 10) { | |
| 269 if (!record->hasUserDeclaredConstructor()) { | |
| 270 emitWarning(record_location, | |
| 271 "Complex class/struct needs an explicit out-of-line " | |
| 272 "constructor."); | |
| 273 } else { | |
| 274 // Iterate across all the constructors in this file and yell if we | |
| 275 // find one that tries to be inline. | |
| 276 for (CXXRecordDecl::ctor_iterator it = record->ctor_begin(); | |
| 277 it != record->ctor_end(); ++it) { | |
| 278 if (it->hasInlineBody()) { | |
| 279 if (it->isCopyConstructor() && | |
| 280 !record->hasUserDeclaredCopyConstructor()) { | |
| 281 emitWarning(record_location, | |
| 282 "Complex class/struct needs an explicit out-of-line " | |
| 283 "copy constructor."); | |
| 284 } else { | |
| 285 emitWarning(it->getInnerLocStart(), | |
| 286 "Complex constructor has an inlined body."); | |
| 287 } | |
| 288 } | |
| 289 } | |
| 290 } | |
| 291 } | |
| 292 | |
| 293 // The destructor side is equivalent except that we don't check for | |
| 294 // trivial members; 20 ints don't need a destructor. | |
| 295 if (dtor_score >= 10 && !record->hasTrivialDestructor()) { | |
| 296 if (!record->hasUserDeclaredDestructor()) { | |
| 297 emitWarning( | |
| 298 record_location, | |
| 299 "Complex class/struct needs an explicit out-of-line " | |
| 300 "destructor."); | |
| 301 } else if (CXXDestructorDecl* dtor = record->getDestructor()) { | |
| 302 if (dtor->hasInlineBody()) { | |
| 303 emitWarning(dtor->getInnerLocStart(), | |
| 304 "Complex destructor has an inline body."); | |
| 305 } | |
| 306 } | |
| 307 } | |
| 308 } | |
| 309 | |
| 310 void CheckVirtualMethod(const CXXMethodDecl* method, | |
| 311 bool warn_on_inline_bodies) { | |
| 312 if (!method->isVirtual()) | |
| 313 return; | |
| 314 | |
| 315 if (!method->isVirtualAsWritten()) { | |
| 316 SourceLocation loc = method->getTypeSpecStartLoc(); | |
| 317 if (isa<CXXDestructorDecl>(method)) | |
| 318 loc = method->getInnerLocStart(); | |
| 319 SourceManager& manager = instance().getSourceManager(); | |
| 320 FullSourceLoc full_loc(loc, manager); | |
| 321 SourceLocation spelling_loc = manager.getSpellingLoc(loc); | |
| 322 diagnostic().Report(full_loc, diag_method_requires_virtual_) | |
| 323 << FixItHint::CreateInsertion(spelling_loc, "virtual "); | |
| 324 } | |
| 325 | |
| 326 // Virtual methods should not have inline definitions beyond "{}". This | |
| 327 // only matters for header files. | |
| 328 if (warn_on_inline_bodies && method->hasBody() && | |
| 329 method->hasInlineBody()) { | |
| 330 if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) { | |
| 331 if (cs->size()) { | |
| 332 emitWarning( | |
| 333 cs->getLBracLoc(), | |
| 334 "virtual methods with non-empty bodies shouldn't be " | |
| 335 "declared inline."); | |
| 336 } | |
| 337 } | |
| 338 } | |
| 339 } | |
| 340 | |
| 341 bool InTestingNamespace(const Decl* record) { | |
| 342 return GetNamespace(record).find("testing") != std::string::npos; | |
| 343 } | |
| 344 | |
| 345 bool IsMethodInBannedOrTestingNamespace(const CXXMethodDecl* method) { | |
| 346 if (InBannedNamespace(method)) | |
| 347 return true; | |
| 348 for (CXXMethodDecl::method_iterator i = method->begin_overridden_methods(); | |
| 349 i != method->end_overridden_methods(); | |
| 350 ++i) { | |
| 351 const CXXMethodDecl* overridden = *i; | |
| 352 if (IsMethodInBannedOrTestingNamespace(overridden) || | |
| 353 InTestingNamespace(overridden)) { | |
| 354 return true; | |
| 355 } | |
| 356 } | |
| 357 | |
| 358 return false; | |
| 359 } | |
| 360 | |
| 361 void CheckOverriddenMethod(const CXXMethodDecl* method) { | |
| 362 if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>()) | |
| 363 return; | |
| 364 | |
| 365 if (isa<CXXDestructorDecl>(method) || method->isPure()) | |
| 366 return; | |
| 367 | |
| 368 if (IsMethodInBannedOrTestingNamespace(method)) | |
| 369 return; | |
| 370 | |
| 371 SourceManager& manager = instance().getSourceManager(); | |
| 372 SourceRange type_info_range = | |
| 373 method->getTypeSourceInfo()->getTypeLoc().getSourceRange(); | |
| 374 FullSourceLoc loc(type_info_range.getBegin(), manager); | |
| 375 | |
| 376 // Build the FixIt insertion point after the end of the method definition, | |
| 377 // including any const-qualifiers and attributes, and before the opening | |
| 378 // of the l-curly-brace (if inline) or the semi-color (if a declaration). | |
| 379 SourceLocation spelling_end = | |
| 380 manager.getSpellingLoc(type_info_range.getEnd()); | |
| 381 if (spelling_end.isValid()) { | |
| 382 SourceLocation token_end = Lexer::getLocForEndOfToken( | |
| 383 spelling_end, 0, manager, LangOptions()); | |
| 384 diagnostic().Report(token_end, diag_method_requires_override_) | |
| 385 << FixItHint::CreateInsertion(token_end, " OVERRIDE"); | |
| 386 } else { | |
| 387 diagnostic().Report(loc, diag_method_requires_override_); | |
| 388 } | |
| 389 } | |
| 390 | |
| 391 // Makes sure there is a "virtual" keyword on virtual methods. | |
| 392 // | |
| 393 // Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a | |
| 394 // trick to get around that. If a class has member variables whose types are | |
| 395 // in the "testing" namespace (which is how gmock works behind the scenes), | |
| 396 // there's a really high chance we won't care about these errors | |
| 397 void CheckVirtualMethods(SourceLocation record_location, | |
| 398 CXXRecordDecl* record, | |
| 399 bool warn_on_inline_bodies) { | |
| 400 for (CXXRecordDecl::field_iterator it = record->field_begin(); | |
| 401 it != record->field_end(); ++it) { | |
| 402 CXXRecordDecl* record_type = | |
| 403 it->getTypeSourceInfo()->getTypeLoc().getTypePtr()-> | |
| 404 getAsCXXRecordDecl(); | |
| 405 if (record_type) { | |
| 406 if (InTestingNamespace(record_type)) { | |
| 407 return; | |
| 408 } | |
| 409 } | |
| 410 } | |
| 411 | |
| 412 for (CXXRecordDecl::method_iterator it = record->method_begin(); | |
| 413 it != record->method_end(); ++it) { | |
| 414 if (it->isCopyAssignmentOperator() || isa<CXXConstructorDecl>(*it)) { | |
| 415 // Ignore constructors and assignment operators. | |
| 416 } else if (isa<CXXDestructorDecl>(*it) && | |
| 417 !record->hasUserDeclaredDestructor()) { | |
| 418 // Ignore non-user-declared destructors. | |
| 419 } else { | |
| 420 CheckVirtualMethod(*it, warn_on_inline_bodies); | |
| 421 CheckOverriddenMethod(*it); | |
| 422 } | |
| 423 } | |
| 424 } | |
| 425 | |
| 426 void CountType(const Type* type, | |
| 427 int* trivial_member, | |
| 428 int* non_trivial_member, | |
| 429 int* templated_non_trivial_member) { | |
| 430 switch (type->getTypeClass()) { | |
| 431 case Type::Record: { | |
| 432 // Simplifying; the whole class isn't trivial if the dtor is, but | |
| 433 // we use this as a signal about complexity. | |
| 434 if (TypeHasNonTrivialDtor(type)) | |
| 435 (*trivial_member)++; | |
| 436 else | |
| 437 (*non_trivial_member)++; | |
| 438 break; | |
| 439 } | |
| 440 case Type::TemplateSpecialization: { | |
| 441 TemplateName name = | |
| 442 dyn_cast<TemplateSpecializationType>(type)->getTemplateName(); | |
| 443 bool whitelisted_template = false; | |
| 444 | |
| 445 // HACK: I'm at a loss about how to get the syntax checker to get | |
| 446 // whether a template is exterened or not. For the first pass here, | |
| 447 // just do retarded string comparisons. | |
| 448 if (TemplateDecl* decl = name.getAsTemplateDecl()) { | |
| 449 std::string base_name = decl->getNameAsString(); | |
| 450 if (base_name == "basic_string") | |
| 451 whitelisted_template = true; | |
| 452 } | |
| 453 | |
| 454 if (whitelisted_template) | |
| 455 (*non_trivial_member)++; | |
| 456 else | |
| 457 (*templated_non_trivial_member)++; | |
| 458 break; | |
| 459 } | |
| 460 case Type::Elaborated: { | |
| 461 CountType( | |
| 462 dyn_cast<ElaboratedType>(type)->getNamedType().getTypePtr(), | |
| 463 trivial_member, non_trivial_member, templated_non_trivial_member); | |
| 464 break; | |
| 465 } | |
| 466 case Type::Typedef: { | |
| 467 while (const TypedefType* TT = dyn_cast<TypedefType>(type)) { | |
| 468 type = TT->getDecl()->getUnderlyingType().getTypePtr(); | |
| 469 } | |
| 470 CountType(type, trivial_member, non_trivial_member, | |
| 471 templated_non_trivial_member); | |
| 472 break; | |
| 473 } | |
| 474 default: { | |
| 475 // Stupid assumption: anything we see that isn't the above is one of | |
| 476 // the 20 integer types. | |
| 477 (*trivial_member)++; | |
| 478 break; | |
| 479 } | |
| 480 } | |
| 481 } | |
| 482 | |
| 483 // Check |record| for issues that are problematic for ref-counted types. | |
| 484 // Note that |record| may not be a ref-counted type, but a base class for | |
| 485 // a type that is. | |
| 486 // If there are issues, update |loc| with the SourceLocation of the issue | |
| 487 // and returns appropriately, or returns None if there are no issues. | |
| 488 static RefcountIssue CheckRecordForRefcountIssue( | |
| 489 const CXXRecordDecl* record, | |
| 490 SourceLocation &loc) { | |
| 491 if (!record->hasUserDeclaredDestructor()) { | |
| 492 loc = record->getLocation(); | |
| 493 return ImplicitDestructor; | |
| 494 } | |
| 495 | |
| 496 if (CXXDestructorDecl* dtor = record->getDestructor()) { | |
| 497 if (dtor->getAccess() == AS_public) { | |
| 498 loc = dtor->getInnerLocStart(); | |
| 499 return PublicDestructor; | |
| 500 } | |
| 501 } | |
| 502 | |
| 503 return None; | |
| 504 } | |
| 505 | |
| 506 // Adds either a warning or error, based on the current handling of | |
| 507 // -Werror. | |
| 508 DiagnosticsEngine::Level getErrorLevel() { | |
| 509 return diagnostic().getWarningsAsErrors() ? | |
| 510 DiagnosticsEngine::Error : DiagnosticsEngine::Warning; | |
| 511 } | |
| 512 | |
| 513 // Returns true if |base| specifies one of the Chromium reference counted | |
| 514 // classes (base::RefCounted / base::RefCountedThreadSafe). | |
| 515 static bool IsRefCountedCallback(const CXXBaseSpecifier* base, | |
| 516 CXXBasePath& path, | |
| 517 void* user_data) { | |
| 518 FindBadConstructsConsumer* self = | |
| 519 static_cast<FindBadConstructsConsumer*>(user_data); | |
| 520 | |
| 521 const TemplateSpecializationType* base_type = | |
| 522 dyn_cast<TemplateSpecializationType>( | |
| 523 UnwrapType(base->getType().getTypePtr())); | |
| 524 if (!base_type) { | |
| 525 // Base-most definition is not a template, so this cannot derive from | |
| 526 // base::RefCounted. However, it may still be possible to use with a | |
| 527 // scoped_refptr<> and support ref-counting, so this is not a perfect | |
| 528 // guarantee of safety. | |
| 529 return false; | |
| 530 } | |
| 531 | |
| 532 TemplateName name = base_type->getTemplateName(); | |
| 533 if (TemplateDecl* decl = name.getAsTemplateDecl()) { | |
| 534 std::string base_name = decl->getNameAsString(); | |
| 535 | |
| 536 // Check for both base::RefCounted and base::RefCountedThreadSafe. | |
| 537 if (base_name.compare(0, 10, "RefCounted") == 0 && | |
| 538 self->GetNamespace(decl) == "base") { | |
| 539 return true; | |
| 540 } | |
| 541 } | |
| 542 | |
| 543 return false; | |
| 544 } | |
| 545 | |
| 546 // Returns true if |base| specifies a class that has a public destructor, | |
| 547 // either explicitly or implicitly. | |
| 548 static bool HasPublicDtorCallback(const CXXBaseSpecifier* base, | |
| 549 CXXBasePath& path, | |
| 550 void* user_data) { | |
| 551 // Only examine paths that have public inheritance, as they are the | |
| 552 // only ones which will result in the destructor potentially being | |
| 553 // exposed. This check is largely redundant, as Chromium code should be | |
| 554 // exclusively using public inheritance. | |
| 555 if (path.Access != AS_public) | |
| 556 return false; | |
| 557 | |
| 558 CXXRecordDecl* record = dyn_cast<CXXRecordDecl>( | |
| 559 base->getType()->getAs<RecordType>()->getDecl()); | |
| 560 SourceLocation unused; | |
| 561 return None != CheckRecordForRefcountIssue(record, unused); | |
| 562 } | |
| 563 | |
| 564 // Outputs a C++ inheritance chain as a diagnostic aid. | |
| 565 void PrintInheritanceChain(const CXXBasePath& path) { | |
| 566 for (CXXBasePath::const_iterator it = path.begin(); it != path.end(); | |
| 567 ++it) { | |
| 568 diagnostic().Report(it->Base->getLocStart(), diag_note_inheritance_) | |
| 569 << it->Class << it->Base->getType(); | |
| 570 } | |
| 571 } | |
| 572 | |
| 573 unsigned DiagnosticForIssue(RefcountIssue issue) { | |
| 574 switch (issue) { | |
| 575 case ImplicitDestructor: | |
| 576 return diag_no_explicit_dtor_; | |
| 577 case PublicDestructor: | |
| 578 return diag_public_dtor_; | |
| 579 case None: | |
| 580 assert(false && "Do not call DiagnosticForIssue with issue None"); | |
| 581 return 0; | |
| 582 } | |
| 583 assert(false); | |
| 584 return 0; | |
| 585 } | |
| 586 | |
| 587 // Check |record| to determine if it has any problematic refcounting | |
| 588 // issues and, if so, print them as warnings/errors based on the current | |
| 589 // value of getErrorLevel(). | |
| 590 // | |
| 591 // If |record| is a C++ class, and if it inherits from one of the Chromium | |
| 592 // ref-counting classes (base::RefCounted / base::RefCountedThreadSafe), | |
| 593 // ensure that there are no public destructors in the class hierarchy. This | |
| 594 // is to guard against accidentally stack-allocating a RefCounted class or | |
| 595 // sticking it in a non-ref-counted container (like scoped_ptr<>). | |
| 596 void CheckRefCountedDtors(SourceLocation record_location, | |
| 597 CXXRecordDecl* record) { | |
| 598 // Skip anonymous structs. | |
| 599 if (record->getIdentifier() == NULL) | |
| 600 return; | |
| 601 | |
| 602 // Determine if the current type is even ref-counted. | |
| 603 CXXBasePaths refcounted_path; | |
| 604 if (!record->lookupInBases( | |
| 605 &FindBadConstructsConsumer::IsRefCountedCallback, this, | |
| 606 refcounted_path)) { | |
| 607 return; // Class does not derive from a ref-counted base class. | |
| 608 } | |
| 609 | |
| 610 // Easy check: Check to see if the current type is problematic. | |
| 611 SourceLocation loc; | |
| 612 RefcountIssue issue = CheckRecordForRefcountIssue(record, loc); | |
| 613 if (issue != None) { | |
| 614 diagnostic().Report(loc, DiagnosticForIssue(issue)); | |
| 615 PrintInheritanceChain(refcounted_path.front()); | |
| 616 return; | |
| 617 } | |
| 618 if (CXXDestructorDecl* dtor = | |
| 619 refcounted_path.begin()->back().Class->getDestructor()) { | |
| 620 if (dtor->getAccess() == AS_protected && | |
| 621 !dtor->isVirtual()) { | |
| 622 loc = dtor->getInnerLocStart(); | |
| 623 diagnostic().Report(loc, diag_protected_non_virtual_dtor_); | |
| 624 return; | |
| 625 } | |
| 626 } | |
| 627 | |
| 628 // Long check: Check all possible base classes for problematic | |
| 629 // destructors. This checks for situations involving multiple | |
| 630 // inheritance, where the ref-counted class may be implementing an | |
| 631 // interface that has a public or implicit destructor. | |
| 632 // | |
| 633 // struct SomeInterface { | |
| 634 // virtual void DoFoo(); | |
| 635 // }; | |
| 636 // | |
| 637 // struct RefCountedInterface | |
| 638 // : public base::RefCounted<RefCountedInterface>, | |
| 639 // public SomeInterface { | |
| 640 // private: | |
| 641 // friend class base::Refcounted<RefCountedInterface>; | |
| 642 // virtual ~RefCountedInterface() {} | |
| 643 // }; | |
| 644 // | |
| 645 // While RefCountedInterface is "safe", in that its destructor is | |
| 646 // private, it's possible to do the following "unsafe" code: | |
| 647 // scoped_refptr<RefCountedInterface> some_class( | |
| 648 // new RefCountedInterface); | |
| 649 // // Calls SomeInterface::~SomeInterface(), which is unsafe. | |
| 650 // delete static_cast<SomeInterface*>(some_class.get()); | |
| 651 if (!options_.check_base_classes) | |
| 652 return; | |
| 653 | |
| 654 // Find all public destructors. This will record the class hierarchy | |
| 655 // that leads to the public destructor in |dtor_paths|. | |
| 656 CXXBasePaths dtor_paths; | |
| 657 if (!record->lookupInBases( | |
| 658 &FindBadConstructsConsumer::HasPublicDtorCallback, this, | |
| 659 dtor_paths)) { | |
| 660 return; | |
| 661 } | |
| 662 | |
| 663 for (CXXBasePaths::const_paths_iterator it = dtor_paths.begin(); | |
| 664 it != dtor_paths.end(); ++it) { | |
| 665 // The record with the problem will always be the last record | |
| 666 // in the path, since it is the record that stopped the search. | |
| 667 const CXXRecordDecl* problem_record = dyn_cast<CXXRecordDecl>( | |
| 668 it->back().Base->getType()->getAs<RecordType>()->getDecl()); | |
| 669 | |
| 670 issue = CheckRecordForRefcountIssue(problem_record, loc); | |
| 671 | |
| 672 if (issue == ImplicitDestructor) { | |
| 673 diagnostic().Report(record_location, diag_no_explicit_dtor_); | |
| 674 PrintInheritanceChain(refcounted_path.front()); | |
| 675 diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record; | |
| 676 PrintInheritanceChain(*it); | |
| 677 } else if (issue == PublicDestructor) { | |
| 678 diagnostic().Report(record_location, diag_public_dtor_); | |
| 679 PrintInheritanceChain(refcounted_path.front()); | |
| 680 diagnostic().Report(loc, diag_note_public_dtor_); | |
| 681 PrintInheritanceChain(*it); | |
| 682 } | |
| 683 } | |
| 684 } | |
| 685 | |
| 686 // Check for any problems with WeakPtrFactory class members. This currently | |
| 687 // only checks that any WeakPtrFactory<T> member of T appears as the last | |
| 688 // data member in T. We could consider checking for bad uses of | |
| 689 // WeakPtrFactory to refer to other data members, but that would require | |
| 690 // looking at the initializer list in constructors to see what the factory | |
| 691 // points to. | |
| 692 // Note, if we later add other unrelated checks of data members, we should | |
| 693 // consider collapsing them in to one loop to avoid iterating over the data | |
| 694 // members more than once. | |
| 695 void CheckWeakPtrFactoryMembers(SourceLocation record_location, | |
| 696 CXXRecordDecl* record) { | |
| 697 // Skip anonymous structs. | |
| 698 if (record->getIdentifier() == NULL) | |
| 699 return; | |
| 700 | |
| 701 // Iterate through members of the class. | |
| 702 RecordDecl::field_iterator iter(record->field_begin()), | |
| 703 the_end(record->field_end()); | |
| 704 SourceLocation weak_ptr_factory_location; // Invalid initially. | |
| 705 for (; iter != the_end; ++iter) { | |
| 706 // If we enter the loop but have already seen a matching WeakPtrFactory, | |
| 707 // it means there is at least one member after the factory. | |
| 708 if (weak_ptr_factory_location.isValid()) { | |
| 709 diagnostic().Report(weak_ptr_factory_location, | |
| 710 diag_weak_ptr_factory_order_); | |
| 711 } | |
| 712 const TemplateSpecializationType* template_spec_type = | |
| 713 iter->getType().getTypePtr()->getAs<TemplateSpecializationType>(); | |
| 714 if (template_spec_type) { | |
| 715 const TemplateDecl* template_decl = | |
| 716 template_spec_type->getTemplateName().getAsTemplateDecl(); | |
| 717 if (template_decl && template_spec_type->getNumArgs() >= 1) { | |
| 718 if (template_decl->getNameAsString().compare("WeakPtrFactory") == 0 && | |
| 719 GetNamespace(template_decl) == "base") { | |
| 720 const TemplateArgument& arg = template_spec_type->getArg(0); | |
| 721 if (arg.getAsType().getTypePtr()->getAsCXXRecordDecl() == | |
| 722 record->getTypeForDecl()->getAsCXXRecordDecl()) { | |
| 723 weak_ptr_factory_location = iter->getLocation(); | |
| 724 } | |
| 725 } | |
| 726 } | |
| 727 } | |
| 728 } | |
| 729 } | |
| 730 }; | |
| 731 | |
| 732 | |
| 733 class FindBadConstructsAction : public PluginASTAction { | |
| 734 public: | |
| 735 FindBadConstructsAction() { | |
| 736 } | |
| 737 | |
| 738 protected: | |
| 739 // Overridden from PluginASTAction: | |
| 740 virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance, | |
| 741 llvm::StringRef ref) { | |
| 742 return new FindBadConstructsConsumer(instance, options_); | |
| 743 } | |
| 744 | |
| 745 virtual bool ParseArgs(const CompilerInstance& instance, | |
| 746 const std::vector<std::string>& args) { | |
| 747 bool parsed = true; | |
| 748 | |
| 749 for (size_t i = 0; i < args.size() && parsed; ++i) { | |
| 750 if (args[i] == "skip-virtuals-in-implementations") { | |
| 751 // TODO(rsleevi): Remove this once http://crbug.com/115047 is fixed. | |
| 752 options_.check_virtuals_in_implementations = false; | |
| 753 } else if (args[i] == "check-base-classes") { | |
| 754 // TODO(rsleevi): Remove this once http://crbug.com/123295 is fixed. | |
| 755 options_.check_base_classes = true; | |
| 756 } else if (args[i] == "check-weak-ptr-factory-order") { | |
| 757 // TODO(dmichael): Remove this once http://crbug.com/303818 is fixed. | |
| 758 options_.check_weak_ptr_factory_order = true; | |
| 759 } else if (args[i] == "check-enum-last-value") { | |
| 760 // TODO(tsepez): Enable this by default once http://crbug.com/356815 | |
| 761 // and http://crbug.com/356816 are fixed. | |
| 762 options_.check_enum_last_value = true; | |
| 763 } else { | |
| 764 parsed = false; | |
| 765 llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n"; | |
| 766 } | |
| 767 } | |
| 768 | |
| 769 return parsed; | |
| 770 } | |
| 771 | |
| 772 private: | |
| 773 FindBadConstructsOptions options_; | |
| 774 }; | |
| 775 | |
| 776 } // namespace | |
| 777 | |
| 778 static FrontendPluginRegistry::Add<FindBadConstructsAction> | |
| 779 X("find-bad-constructs", "Finds bad C++ constructs"); | |
| OLD | NEW |
|---|
Chromium Code Reviews
Issue 348543002:
Chrome clang plugin cleanup part 1 of ?? (Closed)
Base URL: svn://svn.chromium.org/chrome/trunk/src