Allow padding checker to traverse simple class hierarchies

[tekknolagi]

Summary

The existing padding checker skips classes that have any base classes. This patch allows the checker to traverse very simple cases: classes that have no fields and have one base class. This is important mostly in the case of array declarations. For example, this case with allowedPad=20:

struct FakeIntSandwich {
  char c1;
  int i;
  char c2;
};

struct AnotherIntSandwich : FakeIntSandwich { // no-warning
};

// But we don't yet support multiple base classes.
struct IntSandwich {};
struct TooManyBaseClasses : FakeIntSandwich, IntSandwich { // no-warning
};

AnotherIntSandwich ais[100];

would not ordinarily have been caught; FakeIntSandwich was not previously padded poorly enough to cause a warning. The previous version of the padding checker saw the declaration of ais of type AnotherIntSandwich[100] and ignored it because it had base classes. With these new changes, the padding checker *will* catch this, having traversed this simple class hierarchy. This test case is included in the file padding_inherit.cpp.

[bcraig]

You should probably add whoever the current code owner of that static analyzer to this review. I have very little involvement with Clang these days, so a "LGTM" from me doesn't carry much weight.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

155–157

1. correctness: If CXXRD->getNumBases() returns 1, this will return true, regardless of the emptiness.

2. style: split this into two if statements. Everything else in this chunk of code _could_ be represented as one massive or-ed together conditional, but instead it uses multiple, back-to-back ifs. This may not make sense after fixing the correctness bug.

168–173

I think this should just be an if, and not an else if.

test/Analysis/padding_inherit.cpp

3–5

Evil test case to add...

struct Empty {};
struct DoubleEmpty : Empty {
    Empty e;
};

I don't think that should warn, as you can't reduce padding by rearranging element.

I guess your new code shouldn't catch that at all anyway, as you are testing in the other direction.... when field-less inherits from field-ed.

[tekknolagi]

Sorry @malcolm.parsons -- I misunderstood code owners. Should not have just looked at a blame of the file...

[tekknolagi]

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

168–173

Changing it to a normal if mucks with the fall-through if pattern: in the old behavior, any empty struct would be skipped. In this new behavior, it shouldn't be the same -- it should only be the empty structs that don't have any base classes, etc.

test/Analysis/padding_inherit.cpp

3–5

I'll add that test case -- certainly something to consider when somebody tackles the harder field/inheritance problem.

[NoQ]

I'll take care of this review as an [analyzer]-responsible person, maybe not immediately because i'm a bit overwhelmed right now - sorry! - and George may want to have a look (also or instead).

[tekknolagi]

Fix confusing & wrong if-statements, add new test suggested by @bcraig

[bcraig]

test/Analysis/padding_cpp.cpp

204–212 ↗

(On Diff #169501)

Looking at this again... what new cases does this catch? FakeIntSandwich was caught before (it is identical to 'PaddedA', and AnotherIntSandwich generated no warning before. So what is different?

EBO1 and EBO2 are cases above that would be nice to catch, but aren't being caught right now.

[george.karpenkov]

@bcraig comments look valid, marking as "Needs Changes"

[tekknolagi]

test/Analysis/padding_cpp.cpp

204–212 ↗

(On Diff #169501)

Ah, you're right. I'll just keep the one in padding_inherit.

[tekknolagi]

Remove useless test case in padding_cpp.cpp

[tekknolagi]

test/Analysis/padding_cpp.cpp

204–212 ↗

(On Diff #169501)

@bcraig I updated the description of the diff to be more informative about the particular cases this change catches.

[tekknolagi]

Make the test case a little easier to read

[bcraig]

add a comment, and it will be fine in my eyes. You'll need signoff from the code owner though.

test/Analysis/padding_cpp.cpp

204–212 ↗

(On Diff #169501)

ok, got it. Both the old and new will catch the exact same class / struct declarations, but the new checker will catch more array cases.

test/Analysis/padding_inherit.cpp

3–5

The comment needs some extra exposition. AllowedPad is set to 20, yet this gets flagged anyway. You should mention that this only warns because of the later declaration of AnotherIntSandwich ais[100];

[tekknolagi]

Ping @NoQ @george.karpenkov

[george.karpenkov]

I'll try to take a look this week.

[NoQ]

Patch looks great, thanks!

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

78–81

This check is already in shouldSkipDecl() (?)

83–85

I guess the TODO is still kinda partially relevant, eg. "TODO: support other combinations of base classes and fields"?

test/Analysis/padding_inherit.cpp

21

Now that's actually interesting: i didn't realize that this checker displays warnings depending on how the structure is *used*. The warning doesn't mention the array, so the user would never figure out why is this a true positive. I guess it'd be great to add an extra note (as in BugReport::addNote()) to this checker's report that'd be attached to the array's location in the code and would say something like note: 'struct FakeIntSandwich' is used within array 'ais' with 100 elements. And also note: 'struct AnotherIntSandwich' inherits from 'struct FakeIntSandwich' at the base specifier.

It's not blocking this patch, just thinking aloud about QoL matters.

[tekknolagi]

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

78–81

Oh yes, you're right.

83–85

Good point, I'll add that.

test/Analysis/padding_inherit.cpp

21

I didn't know of this functionality. I like your suggestion!

[tekknolagi]

Add comments suggested by @NoQ and @bcraig

[NoQ]

Thanks! Also do you have commit access or do you need someone to commit this for you?

[tekknolagi]

@NoQ I think @alexshap will be committing this. Thanks

[tekknolagi]

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

78–81

Actually, I'm not sure if you're right. I think it's necessary here because it's only tested for C++ classes in shouldSkipDecl(). This tests it for C structs too. Either we could lift that outside the C++ section of shouldSkipDecl or repeat it here.

[NoQ]

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

78–81

Hmm, indeed. Sry!

[tekknolagi]

Skip non-definitions in VisitRecord

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

public:
   /// Look for records of overly padded types. If padding *
   /// PadMultiplier exceeds AllowedPad, then generate a report.
   /// PadMultiplier is used to share code with the array padding
   /// checker.
   void visitRecord(const RecordDecl *RD, uint64_t PadMultiplier = 1) const {
     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<CXXRecordDecl>(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()));
 
     CharUnits BaselinePad = calculateBaselinePad(RD, ASTContext, RL);
     if (BaselinePad.isZero())
       return;
 
void visitVariable(const VarDecl *VD) const {
     if (const ConstantArrayType *CArrTy = dyn_cast<ConstantArrayType>(ArrTy))
       Elts = CArrTy->getSize().getZExtValue();
     if (Elts == 0)
       return;
     const RecordType *RT = ArrTy->getElementType()->getAs<RecordType>();
     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);
   }
 
   bool shouldSkipDecl(const RecordDecl *RD) const {
     auto Location = RD->getLocation();
     // If the construct doesn't have a source file, then it's not something
     // we want to diagnose.
     if (!Location.isValid())
       return true;
     SrcMgr::CharacteristicKind Kind =
         BR->getSourceManager().getFileCharacteristic(Location);
     // Throw out all records that come from system headers.
     if (Kind != SrcMgr::C_User)
       return true;
 
     // 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<CXXRecordDecl>(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)
         return true;
       // Can't layout a template, so skip it. We do still layout the
       // instantiations though.
       if (CXXRD->getTypeForDecl()->isDependentType())
         return true;
       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())
         return true;
 
       // Variable length arrays are tricky too.
       QualType Ty = FD->getType();
       if (Ty->isIncompleteArrayType())
         return true;
void reportRecord(
     PathDiagnosticLocation CELoc =
         PathDiagnosticLocation::create(RD, BR->getSourceManager());
     auto Report = llvm::make_unique<BugReport>(*PaddingBug, Os.str(), CELoc);
     Report->setDeclWithIssue(RD);
     Report->addRange(RD->getSourceRange());
     BR->emitReport(std::move(Report));
   }
 };
-}
+} // namespace
 
 void ento::registerPaddingChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<PaddingChecker>();
 }

test/Analysis/padding_inherit.cpp

+// 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;
+};