[PATCH] MS ABI: Diagnose unspecified inheritance model ptrs in the best case

[Nico Weber]

On Thu, Jul 24, 2014 at 12:39 AM, David Majnemer <david.majnemer at gmail.com>
wrote:

> Hi rnk, rsmith,
>
> If the compiler is choosing pointer members using a best-case scheme,
> diagnose when we select the unspecified representation.  It is unlikely
> that this is what the user intended.
>
> http://reviews.llvm.org/D4651
>
> Files:
>   include/clang/Basic/DiagnosticGroups.td
>   include/clang/Basic/DiagnosticSemaKinds.td
>   lib/Sema/SemaDeclAttr.cpp
>   test/SemaCXX/member-pointer-ms.cpp
>
> Index: include/clang/Basic/DiagnosticGroups.td
> ===================================================================
> --- include/clang/Basic/DiagnosticGroups.td
> +++ include/clang/Basic/DiagnosticGroups.td
> @@ -324,6 +324,7 @@
>  def Varargs : DiagGroup<"varargs">;
>
>  def Unsequenced : DiagGroup<"unsequenced">;
> +def UnspecifiedInheritanceModel :
> DiagGroup<"ms-unspecified-inheritance-model">;
>  // GCC name for -Wunsequenced
>  def : DiagGroup<"sequence-point", [Unsequenced]>;
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2505,6 +2505,10 @@
>    InGroup<IgnoredAttributes>;
>  def note_previous_ms_inheritance : Note<
>    "previous inheritance model specified here">;
> +def warn_unspecified_ms_inheritance : Warning<
> +  "the 'full_generality' inheritance model was selected for this class, "
> +  "this will result in very large pointers-to-members">,
>

I would have no idea what to do when the compiler tells me this. ("I
suppose this is true, but so what?") Should there be a note that says what
to do to prevent this? ("define class above this statement or use
__foo_inheritance on declaration" or similar)


> +  InGroup<UnspecifiedInheritanceModel>, DefaultIgnore;
>

I think the usual guidance for warnings is "useful enough to be on by
default, or don't have it". Since this is warning is more about optimizable
code and not about correctness, I think this is ok in this case.


>  def err_machine_mode : Error<"%select{unknown|unsupported}0 machine mode
> %1">;
>  def err_mode_not_primitive : Error<
>    "mode attribute only supported for integer and floating-point types">;
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -3917,6 +3917,14 @@
>      }
>    }
>
> +  // The user probably did not want the unspecified inheritance model in
> the
> +  // best case.  Diagnose this case so that the code could be reworked by
> +  // providing a definition prior to the use of a pointer-to-member or
> via the
> +  // use of inheritance keywords.
> +  if (MSPointerToMemberRepresentationMethod ==
> LangOptions::PPTMK_BestCase &&
> +      SemanticSpelling ==
> MSInheritanceAttr::Keyword_unspecified_inheritance)
> +    Diag(Range.getBegin(), diag::warn_unspecified_ms_inheritance);
> +
>    return ::new (Context)
>        MSInheritanceAttr(Range, Context, BestCase, AttrSpellingListIndex);
>  }
> Index: test/SemaCXX/member-pointer-ms.cpp
> ===================================================================
> --- test/SemaCXX/member-pointer-ms.cpp
> +++ test/SemaCXX/member-pointer-ms.cpp
> @@ -1,5 +1,5 @@
> -// RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only
> -triple=i386-pc-win32 -verify -DVMB %s
> -// RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only
> -triple=x86_64-pc-win32 -verify -DVMB %s
> +// RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only
> -triple=i386-pc-win32 -verify -DVMB -Wms-unspecified-inheritance-model %s
> +// RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only
> -triple=x86_64-pc-win32 -verify -DVMB -Wms-unspecified-inheritance-model %s
>  // RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only
> -triple=x86_64-pc-win32 -verify -DVMV -fms-memptr-rep=virtual %s
>  //
>  // This file should also give no diagnostics when run through cl.exe from
> MSVS
> @@ -167,6 +167,9 @@
>
>  // Re-declare to force us to iterate decls when adding attributes.
>  struct ForwardDecl1;
> +#ifdef VMB
> +// expected-warning at -2 {{the 'full_generality' inheritance model was
> selected}}
> +#endif
>  struct ForwardDecl2;
>
>  typedef int ForwardDecl1::*MemPtr1;
> @@ -212,6 +215,9 @@
>  };
>
>  struct NewUnspecified;
> +#ifdef VMB
> +// expected-warning at -2 {{the 'full_generality' inheritance model was
> selected}}
> +#endif
>  SingleTemplate<void (IncSingle::*)()> tmpl_single;
>  UnspecTemplate<void (NewUnspecified::*)()> tmpl_unspec;
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140724/40904f1f/attachment.html>