[PATCH] MS ABI: Improve selection of an inheritance model

[Aaron Ballman]

On Wed, Jan 15, 2014 at 12:27 PM, David Majnemer
<david.majnemer at gmail.com> wrote:
>
>
> On Wednesday, January 15, 2014, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> Some attribute-related nits below, but aside from those, LGTM (though
>> I would get secondary confirmation).
>>
>> > Index: include/clang/AST/DeclCXX.h
>> > ===================================================================
>> > --- include/clang/AST/DeclCXX.h
>> > +++ include/clang/AST/DeclCXX.h
>> > @@ -261,9 +261,7 @@
>> >  /// The inheritance model to use for member pointers of a given
>> > CXXRecordDecl.
>> >  enum MSInheritanceModel {
>> >    MSIM_Single,
>> > -  MSIM_SinglePolymorphic,
>> >    MSIM_Multiple,
>> > -  MSIM_MultiplePolymorphic,
>> >    MSIM_Virtual,
>> >    MSIM_Unspecified
>> >  };
>> > @@ -1609,6 +1607,7 @@
>> >
>> >    /// \brief Returns the inheritance model used for this record.
>> >    MSInheritanceModel getMSInheritanceModel() const;
>> > +  void setMSInheritanceModel();
>> >
>> >    /// \brief Determine whether this lambda expression was known to be
>> > dependent
>> >    /// at the time it was created, even if its context does not appear
>> > to be
>> > Index: include/clang/Basic/Attr.td
>> > ===================================================================
>> > --- include/clang/Basic/Attr.td
>> > +++ include/clang/Basic/Attr.td
>> > @@ -1354,7 +1354,12 @@
>> >                     Accessor<"IsUnspecified", [Keyword<"">]>];
>> >    // This index is based off the Spellings list and corresponds to the
>> > empty
>> >    // keyword "spelling."
>> > -  let AdditionalMembers = [{static const int UnspecifiedSpellingIndex =
>> > 3;}];
>> > +  let AdditionalMembers = [{
>> > +  static const unsigned SingleSpellingIndex = 0;
>> > +  static const unsigned MultipleSpellingIndex = 1;
>> > +  static const unsigned VirtualSpellingIndex = 2;
>> > +  static const unsigned UnspecifiedSpellingIndex = 3;
>>
>> I'd rather to go great length to avoid exposing these further...
>>
>> let AdditionalMembers = [{
>>   // Note: these correspond to the spelling list index for a reason,
>> so keep them in sync.
>>   enum Type {
>>     Single,
>>     Multiple,
>>     Virtual,
>>     Unspecified
>>   };
>>
>>   static MSInheritanceAttr *Create(SourceRange R, ASTContext &C, Type T) {
>>     return ::new (C) MSInheritanceAttr(R, C, T);
>>   }
>> }];
>>
>> This keeps all of the crazy and fragile in one spot. Once you land
>> that, I can change the usage in SemaType.cpp over to the new style,
>> and remove the old spelling list constant.
>>
>> > +  }];
>> >  }
>> >
>> >  def Unaligned : IgnoredAttr {
>> > Index: lib/AST/MicrosoftCXXABI.cpp
>> > ===================================================================
>> > --- lib/AST/MicrosoftCXXABI.cpp
>> > +++ lib/AST/MicrosoftCXXABI.cpp
>> > @@ -92,7 +92,8 @@
>> >    return false;
>> >  }
>> >
>> > -static MSInheritanceModel MSInheritanceAttrToModel(MSInheritanceAttr
>> > *Attr) {
>> > +static MSInheritanceModel
>> > +MSInheritanceAttrToModel(const MSInheritanceAttr *Attr) {
>> >    if (Attr->IsSingle())
>> >      return MSIM_Single;
>> >    else if (Attr->IsMultiple())
>> > @@ -104,16 +105,44 @@
>> >    return MSIM_Unspecified;
>> >  }
>> >
>> > +static unsigned MSInheritanceModelToSpellingKind(MSInheritanceModel
>> > MSIM) {
>> > +  switch (MSIM) {
>> > +  case MSIM_Single:
>> > +    return MSInheritanceAttr::SingleSpellingIndex;
>> > +  case MSIM_Multiple:
>> > +    return MSInheritanceAttr::MultipleSpellingIndex;
>> > +  case MSIM_Virtual:
>> > +    return MSInheritanceAttr::VirtualSpellingIndex;
>> > +  case MSIM_Unspecified:
>> > +    return MSInheritanceAttr::UnspecifiedSpellingIndex;
>> > +  }
>> > +}
>> > +
>> > +
>> > +static MSInheritanceModel calculateInheritanceModel(const CXXRecordDecl
>> > *RD) {
>> > +  if (!RD->hasDefinition())
>> > +    return MSIM_Unspecified;
>> > +  if (RD->getNumVBases() > 0)
>> > +    return MSIM_Virtual;
>> > +  if (usesMultipleInheritanceModel(RD))
>> > +    return MSIM_Multiple;
>> > +  return MSIM_Single;
>> > +}
>> > +
>> >  MSInheritanceModel CXXRecordDecl::getMSInheritanceModel() const {
>> > -  if (MSInheritanceAttr *IA = this->getAttr<MSInheritanceAttr>())
>> > +  if (MSInheritanceAttr *IA = getAttr<MSInheritanceAttr>())
>> >      return MSInheritanceAttrToModel(IA);
>> > -  // If there was no explicit attribute, the record must be defined
>> > already, and
>> > -  // we can figure out the inheritance model from its other properties.
>> > -  if (this->getNumVBases() > 0)
>> > -    return MSIM_Virtual;
>> > -  if (usesMultipleInheritanceModel(this))
>> > -    return this->isPolymorphic() ? MSIM_MultiplePolymorphic :
>> > MSIM_Multiple;
>> > -  return this->isPolymorphic() ? MSIM_SinglePolymorphic : MSIM_Single;
>> > +
>> > +  return calculateInheritanceModel(this);
>> > +}
>> > +
>> > +void CXXRecordDecl::setMSInheritanceModel() {
>> > +  if (hasAttr<MSInheritanceAttr>())
>> > +    return;
>> > +
>> > +  MSInheritanceModel MSIM = calculateInheritanceModel(this);
>> > +  addAttr(::new (getASTContext()) MSInheritanceAttr(
>> > +      SourceRange(), getASTContext(),
>> > MSInheritanceModelToSpellingKind(MSIM)));
>>
>> This should be using the source range for the record decl (better
>> error reporting if it ever comes up).
>
> I chose not to because I wanted a way to mark the Attr implicit so that
> tooling, AST dumps and diagnostics wouldn't believe there was one in the
> source.  I can change it to use the record decl's range but could we
> (eventually) get a way to say it was manufactured by clang?

That's on "The List" for someday. We have this problem all over the
source base (usually I whine about it when I see a new usage, too).

> P.S. Personally, I think it would be confusing to use the source range of
> this, or any, implicit Attr.  The diagnostics wouldn't really make sense to
> the user.

I can see some logic on that, but then again, at least pointing the
user "here's the thing that's affected" would be a kindness. However,
it could also be argued that most implicit attributes shouldn't cause
errors or warnings in an ideal world.

~Aaron