This is an archive of the discontinued LLVM Phabricator instance.

Adding checker to detect excess padding in records
ClosedPublic

Authored by bcraig on Nov 18 2015, 8:43 AM.

Details

Summary

The intent of this checker is to generate a report for any class / structure that could reduce its padding by reordering the fields. This results in a very noisy checker. To reduce the noise, this checker will currently only warn when the number of bytes over "optimal" is more than 24. This value is configurable with -analyzer-config performance.Padding:AllowedPad=N. Small values of AllowedPad have the potential to generate hundreds of reports, and gigabytes of HTML reports.

The checker searches for padding violations in two main ways. First, it goes record by record. A report is generated if the fields could be reordered in a way that reduces the padding by more than AllowedPad bytes. Second, the checker will generate a report if an array will cause more than AllowedPad padding bytes to be generated.

The record checker currently skips many ABI specific cases. Classes with base classes are skipped because base class tail padding is ABI specific. Bitfields are just plain hard, and duplicating that code seems like a bad idea. VLAs are both uncommon and non-trivial to fix.

The array checker isn't very thorough right now. It only checks to see if the element type's fields could be reordered, and it doesn't recursively check to see if any of the fields' fields could be reordered. At some point in the future, it would be nice if "arrays" could also look at array new usages and malloc patterns that appear to be creating arrays.

Diff Detail

Event Timeline

bcraig updated this revision to Diff 40518.Nov 18 2015, 8:43 AM
bcraig retitled this revision from to Adding checker to detect excess padding in records.
bcraig updated this object.
bcraig added a subscriber: cfe-commits.

I may be mistaken, but this check looks more appropriate for Clang-tidy.

zaks.anna edited edge metadata.Nov 18 2015, 1:18 PM

I may be mistaken, but this check looks more appropriate for Clang-tidy.

This is a syntactic check. Both clang-tidy as well as the clang static analyzer contain this type of checks. If we move all syntactic checks to clang-tidy, the users that use the analyzer but do not use clang-tidy will not receive the warnings. There is an issue in the opposite direction as well.

I may be mistaken, but this check looks more appropriate for Clang-tidy.

This is a syntactic check. Both clang-tidy as well as the clang static analyzer contain this type of checks. If we move all syntactic checks to clang-tidy, the users that use the analyzer but do not use clang-tidy will not receive the warnings. There is an issue in the opposite direction as well.

I am looking into clang-tidy, and if that's the way we need to go, then I'll do it, but I have a preference for this to exist in the static analyzer side of things.

Note that clang-tidy users get this "for free". With no changes to clang tidy code, -checks=clang-analyzer-performance* will get you padding warnings.

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?

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.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
12

Nit: Please, use proper punctuation in comments (in the implementation and test files).

34

This comment is redundant.

49

'Self' is a bit confusing since it's not LocalVisitor but the PaddingChecker class.

57

Why do we need to implement the visitation here? Can PaddingChecker just implement checkASTDecl(const RecordDecl *RD, ...) and checkASTDecl(const VarDecl *RD, ...)?

72

I'd just make the BugReporter and AllowedPad members to avoid passing them around.

89

This would be better expressed with assert(!DiffPad.isNegative() && "DiffPad should not be negative");

98

nit: Please, use doxygen style comments.

115

Should the rest of the function implementation be moved into a subroutine? Looks like copy and paste from visitRecord.

145

Do you get reports from system headers without this check?

210

Are you intentionally not using getTypeInfoDataSizeInChars? Can this lead to false positives? A comment would be helpful.

326

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.

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.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
57

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.

72

I'm fine doing that, but I was under the impression that the checker was supposed to be as close to stateless as possible.

115

I'll make an attempt to do that. The two functions use BaselinePad and OptimalPad a little differently, which complicates things.

145

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...

210

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.

326

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?

bcraig updated this revision to Diff 40659.Nov 19 2015, 8:45 AM
bcraig edited edge metadata.

Addressed the bulk of Anna's review comments.

bcraig marked 15 inline comments as done.Nov 19 2015, 8:47 AM

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?

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.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
73

That's true, but it does not matter for the syntactic checkers much.

146

I wonder why the analyzer diagnostic reporting mechanism does not take care of this.

323

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?

bcraig marked an inline comment as done.Nov 19 2015, 2:24 PM

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.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
323

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.

can be suppressed without breaking ABI by adding explicit padding members.

We should convey the suppression mechanism as part of the diagnostic.

can be suppressed without breaking ABI by adding explicit padding members.

We should convey the suppression mechanism as part of the diagnostic.

I can make the diagnostic longer. Alternatively, is there a way to provide "Note" style diagnostics in the analyzer? If so, then that seems like the best place to put that information.

bcraig updated this revision to Diff 40818.Nov 20 2015, 12:47 PM
bcraig updated this object.

Adding a test to validate non-trivial message components.
Adding recommendations to the diagnostic on how to fix and / or suppress the generated report.
Changing default padding allowance to reduce initial static analysis shock.
An analysis of llvm+clang+compiler-rt now only generates 16 excessive padding reports in index.html.

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?

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
49

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?

71

Could you add a comment explaining what is PadMultiplier and why it is needed?

197

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)
214

Please, add a high-level description of how optimal pad is calculated.

224

The second comment is unclear. Which vector are you talking about here?

I think it would be better to just move the comments to the places where the action is performed; ex std::sort(Fields.begin(), Fields.end()) in this case.

231

Please, pick a more descriptive name.

250

Is NewPad initialized?

bcraig marked 7 inline comments as done.Dec 11 2015, 7:15 AM

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.

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.
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
49

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.

197

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.

250

CharUnits has a default constructor that initializes the value to 0.

bcraig updated this revision to Diff 42524.Dec 11 2015, 7:50 AM
bcraig marked 3 inline comments as done.

Anna Zaks last round of review requests.

With respect to the issues this checker found, I suggest submitting patches for the clang issues that can be fixed. Can the x-macro case be suppressed with the recommended suppression? I'd also submit a patch to gtest. Submitting patches with the fixes provides a good evaluation of new checkers:)

lib/StaticAnalyzer/Checkers/Checkers.td
49

I think Performance should be in the OptIn package.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
194

It's not 100% clear what the "lowest aligned field" is.

196

Punctuation is missing in 4, 5, 6.

200

Extra space before "Track"?

bcraig marked 3 inline comments as done.Dec 14 2015, 11:16 AM

With respect to the issues this checker found, I suggest submitting patches for the clang issues that can be fixed. Can the x-macro case be suppressed with the recommended suppression? I'd also submit a patch to gtest. Submitting patches with the fixes provides a good evaluation of new checkers:)

I will get some NFC changes for the easy fixes in the LLVM and Clang repositories. The x-macro case is in compiler-rt.

The short answer to the x-macro suppression question is "no". The longer answer is "yes, but...". Here are the first few relevant parts of the compiler-rt x-macro...

COMMON_FLAG(
    bool, symbolize, true,
    "If set, use the online symbolizer from common sanitizer runtime to turn "
    "virtual addresses to file/line locations.")
COMMON_FLAG(
    const char *, external_symbolizer_path, 0,
    "Path to external symbolizer. If empty, the tool will search $PATH for "
    "the symbolizer.")

Nothing would break if I reorganized all the "COMMON_FLAG" calls such that the const char * versions came before all the bool versions. It would cause these two options to become more distant in both the code, and in the help. That seems bad.

Alternatively, we could change the other side of the xmacro to generate an anonymous union of TYPE and void *, and that would suppress the warning and work correctly. However, that would increase the amount of dead-space in the object. That seems counter to the goal of the checker.

If there were inline notations for suppressing warnings, then that would be the best way to handle this case.

Note that this kind of issue comes up to a lesser degree in very large hand written classes as well (i.e. not x-macro based). clang::Sema in clang/include/clang/Sema/Sema.h is almost 9000 lines long, and a lot of the members are grouped by usage instead of by alignment. Re-ordering those fields would likely be detrimental to the readability of that class.

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
194

Adjusting the comment. "Round up to at least the smallest field alignment that we currently have"

zaks.anna added inline comments.Dec 14 2015, 11:23 AM
lib/StaticAnalyzer/Checkers/Checkers.td
49

What do you think about this one?

bcraig updated this revision to Diff 42742.Dec 14 2015, 11:31 AM
bcraig marked an inline comment as done.
bcraig marked 2 inline comments as done.Dec 14 2015, 11:33 AM
zaks.anna accepted this revision.Dec 14 2015, 12:51 PM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.Dec 14 2015, 12:51 PM
bcraig closed this revision.Dec 14 2015, 1:44 PM

Submitted to r255545