[PATCH] D14779: Adding checker to detect excess padding in records

[Ben Craig via cfe-commits]

bcraig marked 7 inline comments as done.
bcraig added a comment.

Somehow, I thought I was waiting on Anna, but I missed the Dec 1 update during the Thanksgiving break, and nothing was blocking me.  An updated patch should be posted soon.

In http://reviews.llvm.org/D14779#299895, @zaks.anna wrote:

> > An analysis of llvm+clang+compiler-rt now only generates 16 excessive padding reports in index.html.
>
>
> Should those be fixed or are they all false positives?


All of them are real problems.

- 6 of them would be straightforward to fix
- 9 of them are the exact same (difficult to fix) issue that gets duplicated because the filenames appear different
  - projects/compiler-rt/lib/**tsan**/../sanitizer_common/sanitizer_flags.h vs. projects/compiler-rt/lib/**asan**/../sanitizer_common/sanitizer_flags.h
  - It's a difficult fix because it is a structure where the elements are defined via a preprocessor x-macro.
- 1 is in gtest-internal-inl.h, and would be straightforward to fix, but I don't know if we would because it might be considered external code.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:48
@@ +47,3 @@
+    // The calls to checkAST* from AnalysisConsumer don't end
+    // visit template instantiations or lambda classes.  We
+    // want to visit those, so make our own RecursiveASTVisitor.
----------------
zaks.anna wrote:
> Wording is off here.
> Would it be possible to provide a checkAST method that does visit temple instantiations and lambdas? What if another checker wants the same functionality?
Fixed the wording.
I don't know of a good way to make the Analysis consumer just pass template instantiations and implicit code to only the checkers that want them. 

The root of a template instantiation or implicit code tree could be pretty straightforward, but once the children of those nodes start to be examined, it becomes a lot more difficult to determine if a particular call to malloc (for example) should be dispatched to a particular checker.

I'm not opposed to the idea, but I would expect that kind of change to go in a different patch.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:196
@@ +195,3 @@
+    auto LeftFoldFunc =
+        [&ASTContext, &RL](const CharUnitInfo &Old, const CharUnitInfo &New) {
+          CharUnitInfo Retval;
----------------
zaks.anna wrote:
> Is this doing more than just accumulating the padding for every field in the  record? For example, it seems we could do it with a simple loop the does:
>   PadSum += Current.Offset - (Prev.Offset + Prev.Size)
Switched to raw loop.  

There has been some advice floating around the C++ community for a little while suggesting that developers should avoid raw loops and use STL and STL like algorithms instead.  ( https://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning ).

To be honest though, the raw loop is a lot easier to read (IMO) than the two lambas + functional programming algorithm.  The raw loop is certainly shorter.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:249
@@ +248,3 @@
+    CharUnits NewOffset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0));
+    CharUnits NewPad;
+
----------------
zaks.anna wrote:
> Is NewPad initialized?
CharUnits has a default constructor that initializes the value to 0.


http://reviews.llvm.org/D14779