[PATCH] Insert poisoned paddings between fields in C++ classes.

[Richard Smith]

================
Comment at: include/clang/AST/Decl.h:3266
@@ -3265,1 +3265,3 @@
 
+  /// MayInsertExtraPadding -- Whether we are allowed to insert extra padding
+  /// between fields. These padding are added to help AddressSanitizer detect
----------------
In new code, please use `\brief` instead of repeating the function name.

================
Comment at: include/clang/AST/Decl.h:3269
@@ +3268,3 @@
+  /// intra-object-overflow bugs.
+  bool MayInsertExtraPadding(bool EmitRemark = false) const;
+
----------------
Please use a leading lowercase letter for this function name for consistency with the rest of the class.

================
Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:49-51
@@ -48,1 +48,5 @@
 
+def remark_sanitize_address_insert_extra_padding : Remark<
+    "MayInsertExtraPadding %0: %1">, BackendInfo,
+    InGroup<SanitizeAddressRemarks>;
+
----------------
Remarks are end-user-visible, so should be a bit more friendly than this. Also, this diagnostic should use "%select{list|of|strings}1" rather than putting user-visible strings in the code in Decl.cpp. Something like:

  "-fsanitize-address-field-padding ignored for %0 because it %select{is packed|is a union|is trivially copyable|has a trivial destructor|is standard layout|is in a blacklisted file|is blacklisted}1"

... would seem reasonable. (I don't think it's worth remarking once per class if the reason we're not sanitizing is because the current language is C; in that case, we should warn once for the entire compilation.)

================
Comment at: lib/AST/Decl.cpp:3633
@@ +3632,3 @@
+  // We may be able to relax some of these requirements.
+  const char *ReasonToReject = 0;
+  if (!CXXRD)
----------------
Please use `nullptr` rather than `0`.

================
Comment at: lib/AST/Decl.cpp:3642
@@ +3641,3 @@
+    ReasonToReject = "trivially copyable";
+  else if (CXXRD->hasSimpleDestructor())
+    ReasonToReject = "has simple DTOR";
----------------
`hasSimpleDestructor` still does not seem right; it's a notion internal to how we manage implicit declaration and definition for destructors and shouldn't be used for anything else. Does `hasTrivialDestructor` work for you?

================
Comment at: lib/AST/Decl.cpp:3648
@@ +3647,3 @@
+    ReasonToReject = "blacklisted by src";
+  else if (II && SCL->inSection("field-padding-type", II->getName()))
+    ReasonToReject = "blacklisted by type";
----------------
Blacklisting by identifier doesn't seem ideal; there should probably be some way of specifying a namespace in the blacklist file.

http://reviews.llvm.org/D5687