[PATCH] D53206: Allow padding checker to traverse simple class hierarchies

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi created this revision.
tekknolagi added reviewers: bcraig, NoQ.
tekknolagi added a project: clang.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D53206

Files:
  lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
  test/Analysis/padding_cpp.cpp
  test/Analysis/padding_inherit.cpp


Index: test/Analysis/padding_inherit.cpp
===================================================================
--- /dev/null
+++ test/Analysis/padding_inherit.cpp
@@ -0,0 +1,21 @@
+// 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];
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: 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<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()));
@@ -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,16 @@
     // 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() != 1) ||
+          CXXRD->getNumBases() != 0)
         return true;
       // Virtual bases are complicated, skipping those for now.
       if (CXXRD->getNumVBases() != 0)
@@ -150,6 +165,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 +342,7 @@
     BR->emitReport(std::move(Report));
   }
 };
-}
+} // namespace
 
 void ento::registerPaddingChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<PaddingChecker>();

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits