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

[Richard Smith]

On 16 Oct 2014 11:28, "Kostya Serebryany" <kcc at google.com> wrote:
>
> Addressed all comments except for one
>
> ================
> 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
> ----------------
> rsmith wrote:
> > In new code, please use `\brief` instead of repeating the function name.
> done
>
> ================
> Comment at: include/clang/AST/Decl.h:3269
> @@ +3268,3 @@
> +  /// intra-object-overflow bugs.
> +  bool MayInsertExtraPadding(bool EmitRemark = false) const;
> +
> ----------------
> rsmith wrote:
> > Please use a leading lowercase letter for this function name for
consistency with the rest of the class.
> done
>
> ================
> 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>;
> +
> ----------------
> rsmith wrote:
> > 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.)
> Done except for "we should warn once for the entire compilation for C"
> What is the kosher way to achieve that in clang?
> (I assume that a function-scope static boolean is not kosher)

Maybe issue a warning from the driver if the flag is specified in a non-c++
compile.

> ================
> 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)
> ----------------
> rsmith wrote:
> > Please use `nullptr` rather than `0`.
> Var changed to int.
>
> ================
> Comment at: lib/AST/Decl.cpp:3642
> @@ +3641,3 @@
> +    ReasonToReject = "trivially copyable";
> +  else if (CXXRD->hasSimpleDestructor())
> +    ReasonToReject = "has simple DTOR";
> ----------------
> rsmith wrote:
> > `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?
> Yes, this seems to work,
>
> ================
> 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";
> ----------------
> rsmith wrote:
> > Blacklisting by identifier doesn't seem ideal; there should probably be
some way of specifying a namespace in the blacklist file.
> done (using getQualifiedNameAsString())
>
> http://reviews.llvm.org/D5687
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141016/df11bad6/attachment.html>