Page MenuHomePhabricator

Allow padding checker to traverse simple class hierarchies
ClosedPublic

Authored by tekknolagi on Oct 12 2018, 9:58 AM.

Details

Summary

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. This is important mostly in the case of array declarations. For example, this case with allowedPad=20:

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];

would not ordinarily have been caught; FakeIntSandwich was not previously padded poorly enough to cause a warning. The previous version of the padding checker saw the declaration of ais of type AnotherIntSandwich[100] and ignored it because it had base classes. With these new changes, the padding checker *will* catch this, having traversed this simple class hierarchy. This test case is included in the file padding_inherit.cpp.

Diff Detail

Repository
rC Clang

Event Timeline

tekknolagi created this revision.Oct 12 2018, 9:58 AM

You should probably add whoever the current code owner of that static analyzer to this review. I have very little involvement with Clang these days, so a "LGTM" from me doesn't carry much weight.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
156–158
  1. correctness: If CXXRD->getNumBases() returns 1, this will return true, regardless of the emptiness.
  1. style: split this into two if statements. Everything else in this chunk of code _could_ be represented as one massive or-ed together conditional, but instead it uses multiple, back-to-back ifs. This may not make sense after fixing the correctness bug.
170–175

I think this should just be an if, and not an else if.

test/Analysis/padding_inherit.cpp
3–5

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.

malcolm.parsons resigned from this revision.Oct 12 2018, 2:01 PM
tekknolagi edited reviewers, added: dcoughlin; removed: malcolm.parsons.Oct 12 2018, 2:43 PM

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

tekknolagi added inline comments.Oct 12 2018, 2:47 PM
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
170–175

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.

test/Analysis/padding_inherit.cpp
3–5

I'll add that test case -- certainly something to consider when somebody tackles the harder field/inheritance problem.

I'll take care of this review as an [analyzer]-responsible person, maybe not immediately because i'm a bit overwhelmed right now - sorry! - and George may want to have a look (also or instead).

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

tekknolagi marked 3 inline comments as done.Oct 12 2018, 3:08 PM
bcraig added inline comments.Oct 12 2018, 7:57 PM
test/Analysis/padding_cpp.cpp
204–212 ↗(On Diff #169501)

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.

MTC added a subscriber: MTC.Oct 15 2018, 2:01 AM

@bcraig comments look valid, marking as "Needs Changes"

tekknolagi added inline comments.Oct 22 2018, 11:33 AM
test/Analysis/padding_cpp.cpp
204–212 ↗(On Diff #169501)

Ah, you're right. I'll just keep the one in padding_inherit.

tekknolagi edited the summary of this revision. (Show Details)

Remove useless test case in padding_cpp.cpp

tekknolagi edited the summary of this revision. (Show Details)Oct 22 2018, 11:48 AM
tekknolagi edited the summary of this revision. (Show Details)Oct 22 2018, 12:26 PM
tekknolagi added inline comments.
test/Analysis/padding_cpp.cpp
204–212 ↗(On Diff #169501)

@bcraig I updated the description of the diff to be more informative about the particular cases this change catches.

Make the test case a little easier to read

add a comment, and it will be fine in my eyes. You'll need signoff from the code owner though.

test/Analysis/padding_cpp.cpp
204–212 ↗(On Diff #169501)

ok, got it. Both the old and new will catch the exact same class / struct declarations, but the new checker will catch more array cases.

test/Analysis/padding_inherit.cpp
3–5

The comment needs some extra exposition. AllowedPad is set to 20, yet this gets flagged anyway. You should mention that this only warns because of the later declaration of AnotherIntSandwich ais[100];

I'll try to take a look this week.

NoQ accepted this revision.Oct 25 2018, 3:49 PM

Patch looks great, thanks!

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
78–81

This check is already in shouldSkipDecl() (?)

83–85

I guess the TODO is still kinda partially relevant, eg. "TODO: support other combinations of base classes and fields"?

test/Analysis/padding_inherit.cpp
21

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.

This revision is now accepted and ready to land.Oct 25 2018, 3:49 PM
tekknolagi added inline comments.Oct 25 2018, 5:32 PM
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
78–81

Oh yes, you're right.

83–85

Good point, I'll add that.

test/Analysis/padding_inherit.cpp
21

I didn't know of this functionality. I like your suggestion!

tekknolagi marked 5 inline comments as done.Oct 25 2018, 5:44 PM

Add comments suggested by @NoQ and @bcraig

NoQ accepted this revision.Oct 25 2018, 6:07 PM

Thanks! Also do you have commit access or do you need someone to commit this for you?

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

tekknolagi added inline comments.Oct 26 2018, 11:47 AM
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
78–81

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.

NoQ added inline comments.Oct 26 2018, 11:49 AM
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
78–81

Hmm, indeed. Sry!

Skip non-definitions in VisitRecord

This revision was automatically updated to reflect the committed changes.