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

[Ben Craig via cfe-commits]

bcraig marked an inline comment as done.
bcraig added a comment.

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

> > I have seen plenty of structures where the specific layout was important and couldn't be changed.
>
>
> Can you give specific examples of these? Can we develop heuristics for them?


The previously mentioned flock and flock64 structures are one example.  These structures have ABI significance for that client, as they cross .so / .dll boundaries.  Other general examples would be structures that mimic the layout of on-disk and on-network structures that people use for memcpy based serialization.  I don't know of a good way to make a heuristic for those structures, but the warnings can be suppressed without breaking ABI by adding explicit padding members.

> > 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 it's better to report many fewer issues that are real problems to "advertise" the checker. Once people see it's value, they can lower the threshold. If we report hundreds of issues, it will scare people off.


I will bump the threshold from 8 to 24, then rerun against clang + llvm.  I'm guessing that that will get the number of reports down below 50.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:322
@@ +321,2 @@
+  Mgr.registerChecker<PaddingChecker>();
+}
----------------
zaks.anna wrote:
> I think it is important to check the numbers to make sure that logic does not regress. Maybe you could create one clone for x86 or only test on x86? Is testing on each architecture tests the code you wrote?
I will do a partial fork of these tests on x64 to validate the numbers coming out.  Running an older versions of these tests on multiple platforms alerted me to the craziness of base class layout and tail padding.  Here's an example of the crazy...

  struct Base {
    virtual ~Base() {}
    int i;
    char c;
  };

  struct Derived : public Base {
    char c1;
    short i1;
    char c2;
  };

On some ABI's, the amount of padding in the Derived portion is 2 bytes (optimal 0), and other ABIs, the observed amount is 3 bytes (optimal 3).  My padding calculation code at the time managed to hit my assert that "optimal" was worse than baseline.


http://reviews.llvm.org/D14779