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

[Ben Craig via cfe-commits]

bcraig added a comment.

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

> This is a partial review. I did not look at the padding calculations closely.
>
> Have you run this over codebases other than clang? Are there any false positives?


I ran this over a large C code base, then spot checked the top dozen of so issues.  I haven't seen a "real" false positive with the current implementation.  I have seen plenty of structures where the specific layout was important and couldn't be changed.  I haven't seen any cases where the checker reported excess padding when it wasn't true.

One of the reasons that I do not attempt to handle base classes is because of the fear of false positives.

> > Even with the default of 8, this checker is too noisy to justify turning on by default. Clang+LLVM has 

> 

> >  hundreds of violations.

> 

> 

> How did you pick the default? Should it be higher? 

>  My main concern is that if the checker is too noisy on most codebases, people might just try it once and keep it turned off. Having a higher default will report less issues but they will be more interesting.


The number was picked by looking at the data.  A huge portion of the results were in the general area of 8 bytes or lower.  These generally felt like noisy reports unless I had more specific justification for them (like evidence of an array of the elements).

Should it be higher?  As I get better at detecting arrays, then I think it makes sense to bump the raw value higher.

I think I'm fine with people only running this checker on occasion.  It feels like a profiler in many ways, and the information doesn't go stale.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:56
@@ +55,3 @@
+          : Self(Self), BR(BR), AllowedPad(AllowedPad) {}
+      bool VisitRecordDecl(const RecordDecl *RD) {
+        Self->visitRecord(RD, BR, AllowedPad);
----------------
zaks.anna wrote:
> Why do we need to implement the visitation here? Can PaddingChecker just implement checkASTDecl(const RecordDecl *RD, ...) and checkASTDecl(const VarDecl *RD, ...)?
I wanted to be able to visit template instantiations and the class portion of a lambda, and the AnalysisConsumer class that calls the varios checkAST* functions doesn't do that.  Once I added this custom visitor, I stuck with it for the VarDecl.

I will add a comment mentioning my rationale.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:71
@@ +70,3 @@
+
+  void visitRecord(const RecordDecl *RD, BugReporter &BR,
+                   int64_t AllowedPad) const {
----------------
zaks.anna wrote:
> I'd just make the BugReporter and AllowedPad members to avoid passing them around.
I'm fine doing that, but I was under the impression that the checker was supposed to be as close to stateless as possible.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:114
@@ +113,3 @@
+      return;
+    auto &ASTContext = RD->getASTContext();
+    const ASTRecordLayout &RL = ASTContext.getASTRecordLayout(RD);
----------------
zaks.anna wrote:
> Should the rest of the function implementation be moved into a subroutine? Looks like copy and paste from visitRecord.
I'll make an attempt to do that.  The two functions use BaselinePad and OptimalPad a little differently, which complicates things.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:144
@@ +143,3 @@
+        BR.getSourceManager().getFileCharacteristic(Location);
+    // Throw out all records that come from system headers.
+    if (Kind != SrcMgr::C_User)
----------------
zaks.anna wrote:
> Do you get reports from system headers without this check?
> 
Yes.  Turns out the structures for flock and flock64 have extra padding.  And since those are in commonly included headers, you get that message a lot...

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:209
@@ +208,3 @@
+      std::tie(Info.Size, std::ignore) =
+          ASTContext.getTypeInfoInChars(FD->getType());
+
----------------
zaks.anna wrote:
> Are you intentionally not using getTypeInfoDataSizeInChars? Can this lead to false positives? A comment would be helpful.
It is intentional, and I will add a comment.

In most cases, it doesn't matter what the data size of a field is, just what the aligned size is.  In general, you can't put one object in another's tail padding.  Note that my goal isn't to say how much padding there is in a structure, but how much you can reduce the padding by reordering the fields.  Knowing the tail padding of a structure doesn't further that goal.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:325
@@ +324,3 @@
+
+    Os << " (" << BaselinePad.getQuantity() << " padding bytes, where "
+       << TargetPad.getQuantity() << " is optimal)";
----------------
zaks.anna wrote:
> Why are you not testing for the full message in the tests? It is important to ensure that the information provided here does not regress.
I wanted the tests to be fairly portable.  The specific numbers change a fair amount between x86, x64, and Arm, and Hexagon.  Any recommendation here?  Maybe make a processor specific fork that has the full message?


http://reviews.llvm.org/D14779