Index: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -75,6 +75,19 @@ if (shouldSkipDecl(RD)) return; + // We need to be looking at a definition, not just any pointer to the + // declaration. + if (!(RD = RD->getDefinition())) + return; + + // This is the simplest correct case: a class with no fields and one base + // class. Other cases are more complicated because of how the base classes + // & fields might interact, so we don't bother dealing with them. + if (auto *CXXRD = dyn_cast(RD)) + if (CXXRD->field_empty() && CXXRD->getNumBases() == 1) + return visitRecord(CXXRD->bases().begin()->getType()->getAsRecordDecl(), + PadMultiplier); + auto &ASTContext = RD->getASTContext(); const ASTRecordLayout &RL = ASTContext.getASTRecordLayout(RD); assert(llvm::isPowerOf2_64(RL.getAlignment().getQuantity())); @@ -112,8 +125,7 @@ if (RT == nullptr) return; - // TODO: Recurse into the fields and base classes to see if any - // of those have excess padding. + // TODO: Recurse into the fields to see if they have excess padding. visitRecord(RT->getDecl(), Elts); } @@ -132,13 +144,17 @@ // Not going to attempt to optimize unions. if (RD->isUnion()) return true; - // How do you reorder fields if you haven't got any? - if (RD->field_empty()) - return true; if (auto *CXXRD = dyn_cast(RD)) { + // TODO: Figure out why we are going through declarations and not only + // definitions. + if (!(CXXRD = CXXRD->getDefinition())) + return true; // Tail padding with base classes ends up being very complicated. - // We will skip objects with base classes for now. - if (CXXRD->getNumBases() != 0) + // We will skip objects with base classes for now, unless they do not + // have fields. + if (!CXXRD->field_empty() && CXXRD->getNumBases() != 0) + return true; + if (CXXRD->field_empty() && CXXRD->getNumBases() != 1) return true; // Virtual bases are complicated, skipping those for now. if (CXXRD->getNumVBases() != 0) @@ -150,6 +166,10 @@ if (CXXRD->getTypeForDecl()->isInstantiationDependentType()) return true; } + // How do you reorder fields if you haven't got any? + else if (RD->field_empty()) + return true; + auto IsTrickyField = [](const FieldDecl *FD) -> bool { // Bitfield layout is hard. if (FD->isBitField()) @@ -323,7 +343,7 @@ BR->emitReport(std::move(Report)); } }; -} +} // namespace void ento::registerPaddingChecker(CheckerManager &Mgr) { Mgr.registerChecker(); Index: test/Analysis/padding_cpp.cpp =================================================================== --- test/Analysis/padding_cpp.cpp +++ test/Analysis/padding_cpp.cpp @@ -200,3 +200,17 @@ // expected-warning@+1{{Excessive padding in 'class (lambda}} auto lambda1 = [ c1 = G.c1, i = G.i, c2 = G.c2 ]{}; auto lambda2 = [ i = G.i, c1 = G.c1, c2 = G.c2 ]{}; // no-warning + +// expected-warning@+1{{Excessive padding in 'struct FakeIntSandwich'}} +struct FakeIntSandwich { + char c1; + int i; + char c2; +}; + +struct AnotherIntSandwich : FakeIntSandwich { // no-warning +}; + +// But we don't yet support multiple base classes. +struct TooManyBaseClasses : FakeIntSandwich, IntSandwich { // no-warning +}; Index: test/Analysis/padding_inherit.cpp =================================================================== --- /dev/null +++ test/Analysis/padding_inherit.cpp @@ -0,0 +1,26 @@ +// RUN: %clang_analyze_cc1 -std=c++14 -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=20 -verify %s + +// A class that has no fields and one base class should visit that base class +// instead. +// expected-warning@+1{{Excessive padding in 'struct FakeIntSandwich'}} +struct FakeIntSandwich { + char c1; + int i; + char c2; +}; + +struct AnotherIntSandwich : FakeIntSandwich { // no-warning +}; + +struct IntSandwich {}; + +// But we don't yet support multiple base classes. +struct TooManyBaseClasses : FakeIntSandwich, IntSandwich { // no-warning +}; + +AnotherIntSandwich ais[100]; + +struct Empty {}; +struct DoubleEmpty : Empty { // no-warning + Empty e; +};