[Sema] Fix Modified Type in address_space AttributedType

[leonardchan]

Summary

This is a fix for https://reviews.llvm.org/D51229 where we pass the address_space qualified type as the modified type of an AttributedType. This change now instead wraps the AttributedType with either the address_space qualifier or a DependentAddressSpaceType.

[aaron.ballman]

Missing test cases.

clang/lib/Sema/SemaType.cpp

5763

I think we usually pass the Sema argument first in these helper methods.

[aaron.ballman]

clang/test/Sema/address_space_attribute.cpp

1 ↗

(On Diff #177561)

This test should be moved to the AST directory instead of Sema.

9 ↗

(On Diff #177561)

Can you also add a test using the [[clang::address_space(1)]] spelling and ensure that it is printed properly?

clang/tools/libclang/CXType.cpp

134

This change seems surprising -- if the parsing options say the caller does not want attributed types, why are we returning one anyway for address space?

[leonardchan]

clang/test/Sema/address_space_attribute.cpp

9 ↗

(On Diff #177561)

I do not think address_space has double bracket spelling. Is there a specific attribute under Attr.td or other td file that specifies if an attribute is supported with c++ spelling?

clang/tools/libclang/CXType.cpp

134

This has to do with ensuring clang_getAddressSpace still returns the proper address_space. It does this by essentially checking the qualifiers of the type, which we now attach to the AttributedType whereas before it was attached to the modified type.

This extra condition is necessary for ensuring that calling clang_getAddressSpace points to the qualified AttributedType instead of the unqualified modified type.

[aaron.ballman]

clang/test/Sema/address_space_attribute.cpp

9 ↗

(On Diff #177561)

All attributes with the Clang spelling are given a GNU-style (__attribute__((foo))) spelling and a double-square bracket ([[clang::foo]]) spelling in the clang vendor namespace (for both C++ and C). Attributes with the GCC spelling are similar in that they provide a GNU-style and double-square bracket spelling (in the gnu vendor namespace), though the [[]] spelling is currently only for C++.

clang/tools/libclang/CXType.cpp

134

My fear is that this will be breaking assumptions in third-party code. If someone disables CXTranslationUnit_IncludeAttributedTypes, they are unlikely to expect to receive an AttributedType and may react poorly to it.

[leonardchan]

• Added the CXX11 test

clang/tools/libclang/CXType.cpp

134

Instead check if the type is address_space attributed and apply the qualifiers the modified type.

[aaron.ballman]

clang/tools/libclang/CXType.cpp

134

Sure, they can always modify their code to handle it gracefully, but it's a silent, breaking change to a somewhat stable API which is why I was prodding about it.

After talking with @rsmith, we're thinking the correct change here is to return the attributed type when the user asks for it, but return the equivalent type rather than the modified type if the user doesn't want attribute sugar (for all attributes, not just address spaces). This way, when a user asks for CXType for one of these, they always get a correct type but sometimes it has attribute type sugar and sometimes it doesn't, depending on the parsing options.

This is still a silent, breaking change in behavior, which is unfortunate. It should probably come with a mention in the release notes about the change to the API and some unit tests as well.

[leonardchan]

clang/tools/libclang/CXType.cpp

134

Ok. In the case of qualifiers then, should the equivalent type still contain the address_space qualifiers when creating the AttributedType?

[aaron.ballman]

clang/tools/libclang/CXType.cpp

134

I believe so, yes -- that would ensure it's the minimally desugared type which the type is canonically equivalent to.

[leonardchan]

clang/tools/libclang/CXType.cpp

134

Sorry for the holdup. So for an AddressSpace AttributedType, we attach the qualifiers only to the equivalent type.

As for this though, the only problem I ran into with returning the equivalent type is for AttributedTypes with attr::ObjCKindOf, a test expects returning the modified type which is an ObjCInterface instead of the equivalent type which is an ObjCObject. The test itself just tests printing of a type, but I'm not sure how significant or intentional printing this specific way was.

[aaron.ballman]

clang/tools/libclang/CXType.cpp

134

Which test was failing because of this?

[leonardchan]

clang/tools/libclang/CXType.cpp

134

clang/test/Index/print-type.m

[leonardchan]

clang/tools/libclang/CXType.cpp

134

Specifically just the check:

CHECK: ParmDecl=p:6:36 (Definition) [type=__kindof Foo *] [typekind=ObjCObjectPointer] [canonicaltype=__kindof Foo *] [canonicaltypekind=ObjCObjectPointer] [basetype=Foo] [basekind=ObjCInterface] [isPOD=1] [pointeetype=Foo] [pointeekind=ObjCInterface]

Where the last 2 fields we're instead [pointeetype=__kindof Foo] [pointeekind=ObjCObject] instead of what they are now.

[aaron.ballman]

clang/tools/libclang/CXType.cpp

134

I looked at the review adding the impacted test case and I did not have the impression this was a principled decision so much as "that's what it prints". I suspect we'd be fine removing the hack and always returning the equivalent type, but ObjC is not my area of expertise.

[leonardchan]

clang/tools/libclang/CXType.cpp

134

Done. Is there also a place I should add this to let others know of this change?

[aaron.ballman]

clang/tools/libclang/CXType.cpp

134

Yup, the release notes document (clang\docs\ReleaseNotes.rst) can be updated with some words about the change in behavior.

[aaron.ballman]

This LGTM, but you should wait a bit to see if @rsmith would like to weigh in. If you don't hear back by mid-next week, go ahead and commit.

[leonardchan]

@rsmith Any comments on this patch before submitting?

clang/include/clang/Sema/Sema.h

public:
   QualType BuildReferenceType(QualType T, bool LValueRef,
                               SourceLocation Loc, DeclarationName Entity);
   QualType BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
                           Expr *ArraySize, unsigned Quals,
                           SourceRange Brackets, DeclarationName Entity);
   QualType BuildVectorType(QualType T, Expr *VecSize, SourceLocation AttrLoc);
   QualType BuildExtVectorType(QualType T, Expr *ArraySize,
                               SourceLocation AttrLoc);
+  QualType BuildAddressSpaceAttr(QualType &T, LangAS ASIdx, Expr *AddrSpace,
+                                 SourceLocation AttrLoc);
+
+  /// Same as above, but constructs the AddressSpace index if not provided.
   QualType BuildAddressSpaceAttr(QualType &T, Expr *AddrSpace,
                                  SourceLocation AttrLoc);
 
   bool CheckFunctionReturnType(QualType T, SourceLocation Loc);
 
   /// Build a function type.
   ///
   /// This routine checks the function type according to C++ rules and

clang/lib/AST/Type.cpp

bool AttributedType::isQualifier() const {
   // always part of the canonical type, though.)
   case attr::ObjCGC:
   case attr::ObjCOwnership:
   case attr::ObjCInertUnsafeUnretained:
   case attr::TypeNonNull:
   case attr::TypeNullable:
   case attr::TypeNullUnspecified:
   case attr::LifetimeBound:
+  case attr::AddressSpace:
     return true;
 
   // All other type attributes aren't qualifiers; they rewrite the modified
   // type to be a semantically different type.
   default:
     return false;
   }
 }

clang/lib/AST/TypePrinter.cpp

switch (TC) {
     case Type::DependentAddressSpace:
     case Type::DependentVector:
     case Type::DependentSizedExtVector:
     case Type::Vector:
     case Type::ExtVector:
     case Type::FunctionProto:
     case Type::FunctionNoProto:
     case Type::Paren:
-    case Type::Attributed:
     case Type::PackExpansion:
     case Type::SubstTemplateTypeParm:
       CanPrefixQualifiers = false;
       break;
+
+    case Type::Attributed: {
+      // We still want to print the address_space before the type if it is an
+      // address_space attribute.
+      const auto *AttrTy = cast<AttributedType>(T);
+      CanPrefixQualifiers = AttrTy->getAttrKind() == attr::AddressSpace;
+    }
   }
 
   return CanPrefixQualifiers;
 }
 
 void TypePrinter::printBefore(QualType T, raw_ostream &OS) {
   SplitQualType Split = splitAccordingToPolicy(T, Policy);
 
void TypePrinter::printAttributedBefore(const AttributedType *T,
   // Prefer the macro forms of the GC and ownership qualifiers.
   if (T->getAttrKind() == attr::ObjCGC ||
       T->getAttrKind() == attr::ObjCOwnership)
     return printBefore(T->getEquivalentType(), OS);
 
   if (T->getAttrKind() == attr::ObjCKindOf)
     OS << "__kindof ";
 
+  if (T->getAttrKind() == attr::AddressSpace)
+    printBefore(T->getEquivalentType(), OS);
+  else
     printBefore(T->getModifiedType(), OS);
 
   if (T->isMSTypeSpec()) {
     switch (T->getAttrKind()) {
     default: return;
     case attr::Ptr32: OS << " __ptr32"; break;
     case attr::Ptr64: OS << " __ptr64"; break;
     case attr::SPtr: OS << " __sptr"; break;
     case attr::UPtr: OS << " __uptr"; break;

clang/lib/Sema/SemaType.cpp

ParsedType Sema::ActOnObjCInstanceType(SourceLocation Loc) {
   TypeSourceInfo *TInfo = Context.getTrivialTypeSourceInfo(T, Loc);
   return CreateParsedType(T, TInfo);
 }
 
 //===----------------------------------------------------------------------===//
 // Type Attribute Processing
 //===----------------------------------------------------------------------===//
 
-/// BuildAddressSpaceAttr - Builds a DependentAddressSpaceType if an expression
-/// is uninstantiated. If instantiated it will apply the appropriate address space
-/// to the type. This function allows dependent template variables to be used in
-/// conjunction with the address_space attribute
-QualType Sema::BuildAddressSpaceAttr(QualType &T, Expr *AddrSpace,
-                                     SourceLocation AttrLoc) {
+/// Build an AddressSpace index from a constant expression and diagnose any
+/// errors related to invalid address_spaces. Returns true on successfully
+/// building an AddressSpace index.
+static bool BuildAddressSpaceIndex(Sema &S, LangAS &ASIdx,
+                                   const Expr *AddrSpace,
   if (!AddrSpace->isValueDependent()) {
-
     llvm::APSInt addrSpace(32);
-    if (!AddrSpace->isIntegerConstantExpr(addrSpace, Context)) {
-      Diag(AttrLoc, diag::err_attribute_argument_type)
+    if (!AddrSpace->isIntegerConstantExpr(addrSpace, S.Context)) {
+      S.Diag(AttrLoc, diag::err_attribute_argument_type)
           << "'address_space'" << AANT_ArgumentIntegerConstant
           << AddrSpace->getSourceRange();
-      return QualType();
+      return false;
     }
 
     // Bounds checking.
     if (addrSpace.isSigned()) {
       if (addrSpace.isNegative()) {
-        Diag(AttrLoc, diag::err_attribute_address_space_negative)
+        S.Diag(AttrLoc, diag::err_attribute_address_space_negative)
             << AddrSpace->getSourceRange();
-        return QualType();
+        return false;
       }
       addrSpace.setIsSigned(false);
     }
 
     llvm::APSInt max(addrSpace.getBitWidth());
     max =
         Qualifiers::MaxAddressSpace - (unsigned)LangAS::FirstTargetAddressSpace;
     if (addrSpace > max) {
-      Diag(AttrLoc, diag::err_attribute_address_space_too_high)
+      S.Diag(AttrLoc, diag::err_attribute_address_space_too_high)
           << (unsigned)max.getZExtValue() << AddrSpace->getSourceRange();
-      return QualType();
+      return false;
     }
 
-    LangAS ASIdx =
+    ASIdx =
         getLangASFromTargetAS(static_cast<unsigned>(addrSpace.getZExtValue()));
+    return true;
+  }
+
+  // Default value for DependentAddressSpaceTypes
+  ASIdx = LangAS::Default;
+  return true;
+}
 
+/// BuildAddressSpaceAttr - Builds a DependentAddressSpaceType if an expression
+/// is uninstantiated. If instantiated it will apply the appropriate address
+/// space to the type. This function allows dependent template variables to be
+/// used in conjunction with the address_space attribute
+QualType Sema::BuildAddressSpaceAttr(QualType &T, LangAS ASIdx, Expr *AddrSpace,
+                                     SourceLocation AttrLoc) {
+  if (!AddrSpace->isValueDependent()) {
     // If this type is already address space qualified with a different
     // address space, reject it.
     // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
     // by qualifiers for two or more different address spaces."
     if (T.getAddressSpace() != LangAS::Default) {
       if (T.getAddressSpace() != ASIdx) {
         Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
         return QualType();
QualType Sema::BuildAddressSpaceAttr(QualType &T, LangAS ASIdx, Expr *AddrSpace,
   if (T->getAs<DependentAddressSpaceType>()) {
     Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
     return QualType();
   }
 
   return Context.getDependentAddressSpaceType(T, AddrSpace, AttrLoc);
 }
 
+QualType Sema::BuildAddressSpaceAttr(QualType &T, Expr *AddrSpace,
+                                     SourceLocation AttrLoc) {
+  LangAS ASIdx;
+  if (!BuildAddressSpaceIndex(*this, ASIdx, AddrSpace, AttrLoc))
+    return QualType();
+  return BuildAddressSpaceAttr(T, ASIdx, AddrSpace, AttrLoc);
+}
+
 /// HandleAddressSpaceTypeAttribute - Process an address_space attribute on the
 /// specified type.  The attribute contains 1 argument, the id of the address
 /// space for the type.
 static void HandleAddressSpaceTypeAttribute(QualType &Type,
                                             const ParsedAttr &Attr,
                                             TypeProcessingState &State) {
   Sema &S = State.getSema();
 
if (Attr.isArgIdent(0)) {
       if (AddrSpace.isInvalid())
         return;
 
       ASArgExpr = static_cast<Expr *>(AddrSpace.get());
     } else {
       ASArgExpr = static_cast<Expr *>(Attr.getArgAsExpr(0));
     }
 
-    // Create the DependentAddressSpaceType or append an address space onto
-    // the type.
-    QualType T = S.BuildAddressSpaceAttr(Type, ASArgExpr, Attr.getLoc());
+    LangAS ASIdx;
+    if (!BuildAddressSpaceIndex(S, ASIdx, ASArgExpr, Attr.getLoc())) {
+      Attr.setInvalid();
+      return;
+    }
 
-    if (!T.isNull()) {
-      ASTContext &Ctx = S.Context;
-      auto *ASAttr = ::new (Ctx) AddressSpaceAttr(
-          Attr.getRange(), Ctx, Attr.getAttributeSpellingListIndex(),
-          static_cast<unsigned>(T.getQualifiers().getAddressSpace()));
-      Type = State.getAttributedType(ASAttr, T, T);
-    } else {
+        static_cast<unsigned>(ASIdx));
+
+    // If the expression is not value dependent (not templated), then we can
+    // apply the address space qualifiers just to the equivalent type.
+    // Otherwise, we make an AttributedType with the modified and equivalent
+    // type the same, and wrap it in a DependentAddressSpaceType. When this
+    // dependent type is resolved, the qualifier is added to the equivalent type
+    // later.
+    QualType T;
+    if (!ASArgExpr->isValueDependent()) {
+      QualType EquivType =
+          S.BuildAddressSpaceAttr(Type, ASIdx, ASArgExpr, Attr.getLoc());
+      if (EquivType.isNull()) {
         Attr.setInvalid();
+        return;
+      }
+      T = State.getAttributedType(ASAttr, Type, EquivType);
+    } else {
+      T = State.getAttributedType(ASAttr, Type, Type);
+      T = S.BuildAddressSpaceAttr(T, ASIdx, ASArgExpr, Attr.getLoc());
     }
+
+    if (!T.isNull())
+      Type = T;
+    else
+      Attr.setInvalid();
   } else {
     // The keyword-based type attributes imply which address space to use.
     switch (Attr.getKind()) {
     case ParsedAttr::AT_OpenCLGlobalAddressSpace:
       ASIdx = LangAS::opencl_global; break;
     case ParsedAttr::AT_OpenCLLocalAddressSpace:
       ASIdx = LangAS::opencl_local; break;
     case ParsedAttr::AT_OpenCLConstantAddressSpace:
if (attr.isCXX11Attribute()) {
           state.getSema().Diag(attr.getLoc(),
                                IsTypeAttr
                                    ? diag::warn_gcc_ignores_type_attr
                                    : diag::warn_cxx11_gnu_attribute_on_type)
               << attr.getName();
           if (!IsTypeAttr)
             continue;
         }
-      } else if (TAL != TAL_DeclChunk) {
+      } else if (TAL != TAL_DeclChunk &&
+                 attr.getKind() != ParsedAttr::AT_AddressSpace) {
         // Otherwise, only consider type processing for a C++11 attribute if
         // it's actually been applied to a type.
+        // We also allow C++11 address_space attributes to pass through.
         continue;
       }
     }
 
     // If this is an attribute we can handle, do so now,
     // otherwise, add it to the FnAttrs list for rechaining.
     switch (attr.getKind()) {
     default:

clang/test/AST/address_space_attribute.cpp

+// RUN: %clang_cc1 %s -ast-dump | FileCheck %s
+
+// Veryify the ordering of the address_space attribute still comes before the
+// type whereas other attributes are still printed after.
+
+template <int I>
+void func() {
+  // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *'
+  __attribute__((address_space(1))) int *x;
+
+  // CHECK: VarDecl {{.*}} a 'int * __attribute__((noderef))'
+  int __attribute__((noderef)) * a;
+
+  // CHECK: VarDecl {{.*}} y '__attribute__((address_space(2))) int *'
+  __attribute__((address_space(I))) int *y;
+
+  // CHECK: VarDecl {{.*}} z '__attribute__((address_space(3))) int *'
+  [[clang::address_space(3)]] int *z;
+}
+
+void func2() {
+  func<2>();
+}

clang/tools/libclang/CXType.cpp

 #undef TKCASE
 }
 
 
 CXType cxtype::MakeCXType(QualType T, CXTranslationUnit TU) {
   CXTypeKind TK = CXType_Invalid;
 
   if (TU && !T.isNull()) {
+    ASTContext &Ctx = cxtu::getASTUnit(TU)->getASTContext();
+
     // Handle attributed types as the original type
     if (auto *ATT = T->getAs<AttributedType>()) {
       if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
-        return MakeCXType(ATT->getModifiedType(), TU);
+        // In the event we have an ObjCKindOf, we want to return the modified
+        // type, which is an ObjCInterface instead of the equivalent type, which
+        // is an ObjCObject. Either way, we do not include the attribute.
+        QualType ResultTy = ATT->getAttrKind() == attr::ObjCKindOf
+                                ? ATT->getModifiedType()
+                                : ATT->getEquivalentType();
+        return MakeCXType(ResultTy, TU);
       }
     }
     // Handle paren types as the original type
     if (auto *PTT = T->getAs<ParenType>()) {
       return MakeCXType(PTT->getInnerType(), TU);
     }
 
-    ASTContext &Ctx = cxtu::getASTUnit(TU)->getASTContext();
     if (Ctx.getLangOpts().ObjC) {
       QualType UnqualT = T.getUnqualifiedType();
       if (Ctx.isObjCIdType(UnqualT))
         TK = CXType_ObjCId;
       else if (Ctx.isObjCClassType(UnqualT))
         TK = CXType_ObjCClass;
       else if (Ctx.isObjCSelType(UnqualT))
         TK = CXType_ObjCSel;