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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi updated this revision to Diff 171568.
tekknolagi added a comment.

Skip non-definitions in `VisitRecord`


Repository:
  rC Clang

https://reviews.llvm.org/D53206

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

Index: test/Analysis/padding_inherit.cpp
===
--- /dev/null
+++ test/Analysis/padding_inherit.cpp
@@ -0,0 +1,28 @@
+// 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. Note that despite having excess padding of 2, this is flagged
+// because of its usage in an array of 100 elements below (`ais').
+// TODO: Add a note to the bug report with BugReport::addNote() to mention the
+// variable using the class and also mention what class is inherting from what.
+// 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 IntSandwich {};
+struct TooManyBaseClasses : FakeIntSandwich, IntSandwich { // no-warning
+};
+
+AnotherIntSandwich ais[100];
+
+struct Empty {};
+struct DoubleEmpty : Empty { // no-warning
+Empty e;
+};
Index: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
@@ -75,6 +75,20 @@
 if (shouldSkipDecl(RD))
   return;
 
+// TODO: Figure out why we are going through declarations and not only
+// definitions.
+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.
+// TODO: Support other combinations of base classes and fields.
+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,12 +126,15 @@
 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 {
+// TODO: Figure out why we are going through declarations and not only
+// definitions.
+if (!(RD = RD->getDefinition()))
+  return true;
 auto Location = RD->getLocation();
 // If the construct doesn't have a source file, then it's not something
 // we want to diagnose.
@@ -132,13 +149,14 @@
 // 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)) {
   // 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.
+  // TODO: Handle more base class scenarios.
+  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 +168,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 +345,7 @@
 BR->emitReport(std::move(Report));
   }
 };
-}
+} // namespace
 
 void ento::registerPaddingChecker(CheckerManager &Mgr) {
   Mgr.registerChecker();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:78-81
+// We need to be looking at a definition, not just any pointer to the
+// declaration.
+if (!(RD = RD->getDefinition()))
+  return;

tekknolagi wrote:
> NoQ wrote:
> > This check is already in `shouldSkipDecl()` (?)
> Oh yes, you're right.
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.


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi updated this revision to Diff 171234.
tekknolagi added a comment.

Add comments suggested by @NoQ and @bcraig


Repository:
  rC Clang

https://reviews.llvm.org/D53206

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

Index: test/Analysis/padding_inherit.cpp
===
--- /dev/null
+++ test/Analysis/padding_inherit.cpp
@@ -0,0 +1,28 @@
+// 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. Note that despite having excess padding of 2, this is flagged
+// because of its usage in an array of 100 elements below (`ais').
+// TODO: Add a note to the bug report with BugReport::addNote() to mention the
+// variable using the class and also mention what class is inherting from what.
+// 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 IntSandwich {};
+struct TooManyBaseClasses : FakeIntSandwich, IntSandwich { // no-warning
+};
+
+AnotherIntSandwich ais[100];
+
+struct Empty {};
+struct DoubleEmpty : Empty { // no-warning
+Empty e;
+};
Index: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
@@ -75,6 +75,15 @@
 if (shouldSkipDecl(RD))
   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.
+// TODO: Support other combinations of base classes and fields.
+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 +121,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 +140,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 +162,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 +339,7 @@
 BR->emitReport(std::move(Report));
   }
 };
-}
+} // namespace
 
 void ento::registerPaddingChecker(CheckerManager &Mgr) {
   Mgr.registerChecker();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:78-81
+// We need to be looking at a definition, not just any pointer to the
+// declaration.
+if (!(RD = RD->getDefinition()))
+  return;

NoQ wrote:
> This check is already in `shouldSkipDecl()` (?)
Oh yes, you're right.



Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:83-85
+// 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.

NoQ wrote:
> I guess the TODO is still kinda partially relevant, eg. "TODO: support other 
> combinations of base classes and fields"?
Good point, I'll add that.



Comment at: test/Analysis/padding_inherit.cpp:20
+
+AnotherIntSandwich ais[100];
+

NoQ wrote:
> 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.
I didn't know of this functionality. I like your suggestion!


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added a comment.

Ping @NoQ @george.karpenkov


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi updated this revision to Diff 170475.
tekknolagi added a comment.

Make the test case a little easier to read


Repository:
  rC Clang

https://reviews.llvm.org/D53206

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

Index: test/Analysis/padding_inherit.cpp
===
--- /dev/null
+++ test/Analysis/padding_inherit.cpp
@@ -0,0 +1,25 @@
+// 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
+};
+
+// But we don't yet support multiple base classes.
+struct IntSandwich {};
+struct TooManyBaseClasses : FakeIntSandwich, IntSandwich { // no-warning
+};
+
+AnotherIntSandwich ais[100];
+
+struct Empty {};
+struct DoubleEmpty : Empty { // no-warning
+Empty e;
+};
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();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added inline comments.



Comment at: test/Analysis/padding_cpp.cpp:204-212
+// expected-warning@+1{{Excessive padding in 'struct FakeIntSandwich'}}
+struct FakeIntSandwich {
+  char c1;
+  int i;
+  char c2;
+};
+

tekknolagi wrote:
> bcraig wrote:
> > 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.
> Ah, you're right. I'll just keep the one in `padding_inherit`.
@bcraig I updated the description of the diff to be more informative about the 
particular cases this change catches.


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi updated this revision to Diff 170464.
tekknolagi edited the summary of this revision.
tekknolagi added a comment.

Remove useless test case in `padding_cpp.cpp`


Repository:
  rC Clang

https://reviews.llvm.org/D53206

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

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;
+};
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();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added inline comments.



Comment at: test/Analysis/padding_cpp.cpp:204-212
+// expected-warning@+1{{Excessive padding in 'struct FakeIntSandwich'}}
+struct FakeIntSandwich {
+  char c1;
+  int i;
+  char c2;
+};
+

bcraig wrote:
> 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.
Ah, you're right. I'll just keep the one in `padding_inherit`.


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi updated this revision to Diff 169501.
tekknolagi added a comment.

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


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,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;
+};
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(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();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:167-171
 }
+// How do you reorder fields if you haven't got any?
+else if (RD->field_empty())
+  return true;
+

bcraig wrote:
> I think this should just be an if, and not an else if.
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.



Comment at: test/Analysis/padding_inherit.cpp:2-4
+
+// A class that has no fields and one base class should visit that base class
+// instead.

bcraig wrote:
> 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.
> 
I'll add that test case -- certainly something to consider when somebody 
tackles the harder field/inheritance problem.


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi edited reviewers, added: dcoughlin; removed: malcolm.parsons.
tekknolagi added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D53206



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

[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(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(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();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits