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

[David Majnemer]

On Thu, Jul 24, 2014 at 7:11 AM, Nico Weber <thakis at chromium.org> wrote:

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

The idea is similar to -Wpadded, that diagnostic does not presume to answer
how to minimize the padding in your struct.  Likewise, there are many ways
to silence this diagnostic depending on the TU:
1. Move the definition above the point where in the inheritance model is
selected.
2. Use an inheritance keyword like __single_inheritance.
3. Use #pragma pointers_to_members to select an inheritance model.
4. Use the /vmg and /vm{s,m,v} flags


>
>
>>  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/31461ec4/attachment.html>