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

[Ben Craig via cfe-commits]

bcraig marked 3 inline comments as done.
bcraig added a comment.

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

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


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:194
@@ +193,3 @@
+  /// 3.  If no fields can fit, pad by rounding the current offset up to our
+  ///     lowest aligned field.  Measure and track the amount of padding added.
+  ///     Go back to 2.
----------------
zaks.anna wrote:
> It's not 100% clear what the "lowest aligned field" is.
Adjusting the comment.  "Round up to at least the smallest field alignment that we currently have"


http://reviews.llvm.org/D14779