[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

5747 ↗

(On Diff #177256)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

[aaron.ballman]

clang/tools/libclang/CXType.cpp

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

Which test was failing because of this?

[leonardchan]

clang/tools/libclang/CXType.cpp

132 ↗

(On Diff #177561)

clang/test/Index/print-type.m

[leonardchan]

clang/tools/libclang/CXType.cpp

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

132 ↗

(On Diff #177561)

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

[aaron.ballman]

clang/tools/libclang/CXType.cpp

132 ↗

(On Diff #177561)

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?

cfe/trunk/docs/ReleaseNotes.rst

 ------------
 
 
 - ...
 
 libclang
 --------
 
-...
+- When `CINDEXTEST_INCLUDE_ATTRIBUTED_TYPES` is not provided when making a
+  CXType, the equivalent type of the AttributedType is returned instead of the
+  modified type if the user does not want attribute sugar. The equivalent type
+  represents the minimally-desugared type which the AttributedType is
+  canonically equivalent to.
 
 
 Static Analyzer
 ---------------
 
 - ...
 
 ...

cfe/trunk/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

cfe/trunk/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;
   }
 }

cfe/trunk/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;

cfe/trunk/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 (DiagnoseMultipleAddrSpaceAttributes(*this, T.getAddressSpace(), ASIdx,
                                             AttrLoc))
       return QualType();
 
     return Context.getAddrSpaceQualType(T, ASIdx);
   }
 
   // A check with similar intentions as checking if a type already has an
   // address space except for on a dependent types, basically if the
   // current type is already a DependentAddressSpaceType then its already
   // lined up to have another address space on it and we can't have
   // multiple address spaces on the one pointer indirection
   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.
     ASIdx = Attr.asOpenCLLangAS();
     if (ASIdx == LangAS::Default)
       llvm_unreachable("Invalid address space");
 
     if (DiagnoseMultipleAddrSpaceAttributes(S, Type.getAddressSpace(), ASIdx,
                                             Attr.getLoc())) {
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:

cfe/trunk/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>();
+}

cfe/trunk/test/Index/print-type.m

 // RUN: c-index-test -test-print-type %s | FileCheck %s
 // CHECK: ObjCPropertyDecl=x:2:25 [readonly,] [type=id] [typekind=ObjCId] [canonicaltype=id] [canonicaltypekind=ObjCObjectPointer] [isPOD=1]
 // CHECK: ObjCInstanceMethodDecl=mymethod:3:8 [type=] [typekind=Invalid] [resulttype=int] [resulttypekind=Int] [isPOD=0]
 // CHECK: ObjCInstanceMethodDecl=mymethod2:blah:boo::4:13 [type=] [typekind=Invalid] [resulttype=const id] [resulttypekind=ObjCId] [args= [id] [ObjCId] [Class] [ObjCClass] [SEL] [ObjCSel]] [isPOD=0]
 // CHECK: ParmDecl=z:4:52 (Definition) [type=SEL] [typekind=ObjCSel] [canonicaltype=SEL *] [canonicaltypekind=Pointer] [isPOD=1]
 // CHECK: ObjCInstanceMethodDecl=methodIn:andOut::5:10 (variadic) [Bycopy,] [type=] [typekind=Invalid] [resulttype=id] [resulttypekind=ObjCId] [args= [int] [Int] [short *] [Pointer]] [isPOD=0]
 // CHECK: ParmDecl=i:5:27 (Definition) [In,] [type=int] [typekind=Int] [isPOD=1]
 // CHECK: ParmDecl=j:5:49 (Definition) [Out,] [type=short *] [typekind=Pointer] [isPOD=1] [pointeetype=short] [pointeekind=Short]
-// 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]
+// CHECK: ParmDecl=p:6:36 (Definition) [type=__kindof Foo *] [typekind=ObjCObjectPointer] [canonicaltype=__kindof Foo *] [canonicaltypekind=ObjCObjectPointer] [basetype=Foo] [basekind=ObjCInterface] [isPOD=1] [pointeetype=__kindof Foo] [pointeekind=ObjCObject]
 // CHECK: ObjCPropertyDecl=classProp:7:23 [class,] [type=int] [typekind=Int] [isPOD=1]
 // CHECK: ObjCInstanceMethodDecl=generic:11:12 [type=] [typekind=Invalid] [resulttype=SomeType] [resulttypekind=ObjCTypeParam] [isPOD=0]

cfe/trunk/tools/libclang/CXType.cpp

 
 CXType cxtype::MakeCXType(QualType T, CXTranslationUnit TU) {
   CXTypeKind TK = CXType_Invalid;
 
   if (TU && !T.isNull()) {
     // Handle attributed types as the original type
     if (auto *ATT = T->getAs<AttributedType>()) {
       if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
-        return MakeCXType(ATT->getModifiedType(), TU);
+        // Return the equivalent type which represents the canonically
+        // equivalent type.
+        return MakeCXType(ATT->getEquivalentType(), 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();