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