<p dir="ltr"><br>
On 16 Oct 2014 11:28, "Kostya Serebryany" <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
><br>
> Addressed all comments except for one<br>
><br>
> ================<br>
> Comment at: include/clang/AST/Decl.h:3266<br>
> @@ -3265,1 +3265,3 @@<br>
><br>
> +  /// MayInsertExtraPadding -- Whether we are allowed to insert extra padding<br>
> +  /// between fields. These padding are added to help AddressSanitizer detect<br>
> ----------------<br>
> rsmith wrote:<br>
> > In new code, please use `\brief` instead of repeating the function name.<br>
> done<br>
><br>
> ================<br>
> Comment at: include/clang/AST/Decl.h:3269<br>
> @@ +3268,3 @@<br>
> +  /// intra-object-overflow bugs.<br>
> +  bool MayInsertExtraPadding(bool EmitRemark = false) const;<br>
> +<br>
> ----------------<br>
> rsmith wrote:<br>
> > Please use a leading lowercase letter for this function name for consistency with the rest of the class.<br>
> done<br>
><br>
> ================<br>
> Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:49-51<br>
> @@ -48,1 +48,5 @@<br>
><br>
> +def remark_sanitize_address_insert_extra_padding : Remark<<br>
> +    "MayInsertExtraPadding %0: %1">, BackendInfo,<br>
> +    InGroup<SanitizeAddressRemarks>;<br>
> +<br>
> ----------------<br>
> rsmith wrote:<br>
> > 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:<br>
> ><br>
> >   "-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"<br>
> ><br>
> > ... 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.)<br>
> Done except for "we should warn once for the entire compilation for C"<br>
> What is the kosher way to achieve that in clang?<br>
> (I assume that a function-scope static boolean is not kosher)</p>
<p dir="ltr">Maybe issue a warning from the driver if the flag is specified in a non-c++ compile.</p>
<p dir="ltr">> ================<br>
> Comment at: lib/AST/Decl.cpp:3633<br>
> @@ +3632,3 @@<br>
> +  // We may be able to relax some of these requirements.<br>
> +  const char *ReasonToReject = 0;<br>
> +  if (!CXXRD)<br>
> ----------------<br>
> rsmith wrote:<br>
> > Please use `nullptr` rather than `0`.<br>
> Var changed to int.<br>
><br>
> ================<br>
> Comment at: lib/AST/Decl.cpp:3642<br>
> @@ +3641,3 @@<br>
> +    ReasonToReject = "trivially copyable";<br>
> +  else if (CXXRD->hasSimpleDestructor())<br>
> +    ReasonToReject = "has simple DTOR";<br>
> ----------------<br>
> rsmith wrote:<br>
> > `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?<br>
> Yes, this seems to work,<br>
><br>
> ================<br>
> Comment at: lib/AST/Decl.cpp:3648<br>
> @@ +3647,3 @@<br>
> +    ReasonToReject = "blacklisted by src";<br>
> +  else if (II && SCL->inSection("field-padding-type", II->getName()))<br>
> +    ReasonToReject = "blacklisted by type";<br>
> ----------------<br>
> rsmith wrote:<br>
> > Blacklisting by identifier doesn't seem ideal; there should probably be some way of specifying a namespace in the blacklist file.<br>
> done (using getQualifiedNameAsString())<br>
><br>
> <a href="http://reviews.llvm.org/D5687">http://reviews.llvm.org/D5687</a><br>
><br>
><br>
</p>