[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
This revision was automatically updated to reflect the committed changes.
Closed by commit rL351997: [Sema] Fix Modified Type in address_space
AttributedType (authored by leonardchan, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D55447?vs=182092=183204#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
cfe/trunk/docs/ReleaseNotes.rst
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/Type.cpp
cfe/trunk/lib/AST/TypePrinter.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/AST/address_space_attribute.cpp
cfe/trunk/test/Index/print-type.m
cfe/trunk/tools/libclang/CXType.cpp
Index: cfe/trunk/lib/AST/TypePrinter.cpp
===
--- cfe/trunk/lib/AST/TypePrinter.cpp
+++ cfe/trunk/lib/AST/TypePrinter.cpp
@@ -257,11 +257,17 @@
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(T);
+ CanPrefixQualifiers = AttrTy->getAttrKind() == attr::AddressSpace;
+}
}
return CanPrefixQualifiers;
@@ -1377,7 +1383,10 @@
if (T->getAttrKind() == attr::ObjCKindOf)
OS << "__kindof ";
- printBefore(T->getModifiedType(), OS);
+ if (T->getAttrKind() == attr::AddressSpace)
+printBefore(T->getEquivalentType(), OS);
+ else
+printBefore(T->getModifiedType(), OS);
if (T->isMSTypeSpec()) {
switch (T->getAttrKind()) {
Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -3205,6 +3205,7 @@
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
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -5787,28 +5787,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5817,14 +5816,28 @@
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(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
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan added a comment. @rsmith Any comments on this patch before submitting? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55447/new/ https://reviews.llvm.org/D55447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55447/new/ https://reviews.llvm.org/D55447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 182092.
leonardchan marked an inline comment as done.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/test/Index/print-type.m
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -129,7 +129,9 @@
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
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
Index: clang/test/Index/print-type.m
===
--- clang/test/Index/print-type.m
+++ clang/test/Index/print-type.m
@@ -19,6 +19,6 @@
// 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]
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,23 @@
+// 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
+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>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5756,28 +5756,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> aaron.ballman wrote:
> > leonardchan wrote:
> > > leonardchan wrote:
> > > > aaron.ballman wrote:
> > > > > leonardchan wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > leonardchan wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > 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?
> > > > > > > > > > > > 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.
> > > > > > > > > > > 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.
> > > > > > > > > > Instead check if the type is address_space attributed and
> > > > > > > > > > apply the qualifiers the modified type.
> > > > > > > > > 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.
> > > > > > > > Ok. In the case of qualifiers then, should the equivalent type
> > > > > > > > still contain the address_space qualifiers when creating the
> > > > > > > > AttributedType?
> > > > > > > I believe so, yes -- that would ensure it's the minimally
> > > > > > > desugared type which the type is canonically equivalent to.
> > > > > > 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.
> > > > > Which test was failing because of this?
> > > > clang/test/Index/print-type.m
> > > 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.
> > 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
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan marked an inline comment as done.
leonardchan added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
aaron.ballman wrote:
> leonardchan wrote:
> > leonardchan wrote:
> > > aaron.ballman wrote:
> > > > leonardchan wrote:
> > > > > aaron.ballman wrote:
> > > > > > leonardchan wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > leonardchan wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > 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?
> > > > > > > > > > > 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.
> > > > > > > > > > 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.
> > > > > > > > > Instead check if the type is address_space attributed and
> > > > > > > > > apply the qualifiers the modified type.
> > > > > > > > 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.
> > > > > > > Ok. In the case of qualifiers then, should the equivalent type
> > > > > > > still contain the address_space qualifiers when creating the
> > > > > > > AttributedType?
> > > > > > I believe so, yes -- that would ensure it's the minimally desugared
> > > > > > type which the type is canonically equivalent to.
> > > > > 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.
> > > > Which test was failing because of this?
> > > clang/test/Index/print-type.m
> > 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.
> 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.
Done. Is there also a place I should add this to let others know of this change?
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 181902.
leonardchan marked an inline comment as done.
Herald added a subscriber: arphaman.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/test/Index/print-type.m
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -129,7 +129,9 @@
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
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
Index: clang/test/Index/print-type.m
===
--- clang/test/Index/print-type.m
+++ clang/test/Index/print-type.m
@@ -19,6 +19,6 @@
// 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]
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,23 @@
+// 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
+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>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5756,28 +5756,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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.
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> leonardchan wrote:
> > aaron.ballman wrote:
> > > leonardchan wrote:
> > > > aaron.ballman wrote:
> > > > > leonardchan wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > leonardchan wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > 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?
> > > > > > > > > > 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.
> > > > > > > > > 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.
> > > > > > > > Instead check if the type is address_space attributed and apply
> > > > > > > > the qualifiers the modified type.
> > > > > > > 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.
> > > > > > Ok. In the case of qualifiers then, should the equivalent type
> > > > > > still contain the address_space qualifiers when creating the
> > > > > > AttributedType?
> > > > > I believe so, yes -- that would ensure it's the minimally desugared
> > > > > type which the type is canonically equivalent to.
> > > > 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.
> > > Which test was failing because of this?
> > clang/test/Index/print-type.m
> 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.
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan marked an inline comment as done.
leonardchan added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> aaron.ballman wrote:
> > leonardchan wrote:
> > > aaron.ballman wrote:
> > > > leonardchan wrote:
> > > > > aaron.ballman wrote:
> > > > > > leonardchan wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > leonardchan wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > 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?
> > > > > > > > > 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.
> > > > > > > > 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.
> > > > > > > Instead check if the type is address_space attributed and apply
> > > > > > > the qualifiers the modified type.
> > > > > > 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.
> > > > > Ok. In the case of qualifiers then, should the equivalent type still
> > > > > contain the address_space qualifiers when creating the AttributedType?
> > > > I believe so, yes -- that would ensure it's the minimally desugared
> > > > type which the type is canonically equivalent to.
> > > 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.
> > Which test was failing because of this?
> clang/test/Index/print-type.m
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan marked an inline comment as done.
leonardchan added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
aaron.ballman wrote:
> leonardchan wrote:
> > aaron.ballman wrote:
> > > leonardchan wrote:
> > > > aaron.ballman wrote:
> > > > > leonardchan wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > 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?
> > > > > > > > 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.
> > > > > > > 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.
> > > > > > Instead check if the type is address_space attributed and apply the
> > > > > > qualifiers the modified type.
> > > > > 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.
> > > > Ok. In the case of qualifiers then, should the equivalent type still
> > > > contain the address_space qualifiers when creating the AttributedType?
> > > I believe so, yes -- that would ensure it's the minimally desugared type
> > > which the type is canonically equivalent to.
> > 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.
> Which test was failing because of this?
clang/test/Index/print-type.m
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> aaron.ballman wrote:
> > leonardchan wrote:
> > > aaron.ballman wrote:
> > > > leonardchan wrote:
> > > > > aaron.ballman wrote:
> > > > > > leonardchan wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 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?
> > > > > > > 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.
> > > > > > 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.
> > > > > Instead check if the type is address_space attributed and apply the
> > > > > qualifiers the modified type.
> > > > 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.
> > > Ok. In the case of qualifiers then, should the equivalent type still
> > > contain the address_space qualifiers when creating the AttributedType?
> > I believe so, yes -- that would ensure it's the minimally desugared type
> > which the type is canonically equivalent to.
> 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.
Which test was failing because of this?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 181721.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -129,7 +129,13 @@
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
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
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,23 @@
+// 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
+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>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5756,28 +5756,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5786,14 +5785,28 @@
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(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
+///
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 181720.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -126,10 +126,18 @@
CXTypeKind TK = CXType_Invalid;
if (TU && !T.isNull()) {
+ASTContext = cxtu::getASTUnit(TU)->getASTContext();
+
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
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
@@ -137,7 +145,6 @@
return MakeCXType(PTT->getInnerType(), TU);
}
-ASTContext = cxtu::getASTUnit(TU)->getASTContext();
if (Ctx.getLangOpts().ObjC) {
QualType UnqualT = T.getUnqualifiedType();
if (Ctx.isObjCIdType(UnqualT))
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,23 @@
+// 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
+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>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5756,28 +5756,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5786,14 +5785,28 @@
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;
}
-
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan marked an inline comment as done.
leonardchan added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
aaron.ballman wrote:
> leonardchan wrote:
> > aaron.ballman wrote:
> > > leonardchan wrote:
> > > > aaron.ballman wrote:
> > > > > leonardchan wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 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?
> > > > > > 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.
> > > > > 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.
> > > > Instead check if the type is address_space attributed and apply the
> > > > qualifiers the modified type.
> > > 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.
> > Ok. In the case of qualifiers then, should the equivalent type still
> > contain the address_space qualifiers when creating the AttributedType?
> I believe so, yes -- that would ensure it's the minimally desugared type
> which the type is canonically equivalent to.
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 181719.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -126,10 +126,17 @@
CXTypeKind TK = CXType_Invalid;
if (TU && !T.isNull()) {
+ASTContext = cxtu::getASTUnit(TU)->getASTContext();
+
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
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
@@ -137,7 +144,6 @@
return MakeCXType(PTT->getInnerType(), TU);
}
-ASTContext = cxtu::getASTUnit(TU)->getASTContext();
if (Ctx.getLangOpts().ObjC) {
QualType UnqualT = T.getUnqualifiedType();
if (Ctx.isObjCIdType(UnqualT))
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,23 @@
+// 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
+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>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5756,28 +5756,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5786,14 +5785,28 @@
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 =
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> aaron.ballman wrote:
> > leonardchan wrote:
> > > aaron.ballman wrote:
> > > > leonardchan wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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?
> > > > > 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.
> > > > 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.
> > > Instead check if the type is address_space attributed and apply the
> > > qualifiers the modified type.
> > 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.
> Ok. In the case of qualifiers then, should the equivalent type still contain
> the address_space qualifiers when creating the AttributedType?
I believe so, yes -- that would ensure it's the minimally desugared type which
the type is canonically equivalent to.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan marked an inline comment as done.
leonardchan added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
aaron.ballman wrote:
> leonardchan wrote:
> > aaron.ballman wrote:
> > > leonardchan wrote:
> > > > aaron.ballman wrote:
> > > > > 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?
> > > > 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.
> > > 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.
> > Instead check if the type is address_space attributed and apply the
> > qualifiers the modified type.
> 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.
Ok. In the case of qualifiers then, should the equivalent type still contain
the address_space qualifiers when creating the AttributedType?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> aaron.ballman wrote:
> > leonardchan wrote:
> > > aaron.ballman wrote:
> > > > 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?
> > > 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.
> > 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.
> Instead check if the type is address_space attributed and apply the
> qualifiers the modified type.
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan added inline comments.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
aaron.ballman wrote:
> leonardchan wrote:
> > aaron.ballman wrote:
> > > 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?
> > 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.
> 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.
Instead check if the type is address_space attributed and apply the qualifiers
the modified type.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 177620.
leonardchan marked 2 inline comments as done.
leonardchan added a comment.
- Added the CXX11 test
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -126,10 +126,15 @@
CXTypeKind TK = CXType_Invalid;
if (TU && !T.isNull()) {
+ASTContext = cxtu::getASTUnit(TU)->getASTContext();
+
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
-return MakeCXType(ATT->getModifiedType(), TU);
+QualType ResultTy = ATT->getModifiedType();
+if (ATT->getAttrKind() == attr::AddressSpace)
+ ResultTy = Ctx.getAddrSpaceQualType(ResultTy, T.getAddressSpace());
+return MakeCXType(ResultTy, TU);
}
}
// Handle paren types as the original type
@@ -137,7 +142,6 @@
return MakeCXType(PTT->getInnerType(), TU);
}
-ASTContext = cxtu::getASTUnit(TU)->getASTContext();
if (Ctx.getLangOpts().ObjC) {
QualType UnqualT = T.getUnqualifiedType();
if (Ctx.isObjCIdType(UnqualT))
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,23 @@
+// 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
+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>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5740,28 +5740,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5770,14 +5769,28 @@
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(addrSpace.getZExtValue()));
+return true;
+ }
+ // Default value for
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/test/Sema/address_space_attribute.cpp:9
+ // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *'
+ __attribute__((address_space(1))) int *x;
+
leonardchan wrote:
> aaron.ballman wrote:
> > Can you also add a test using the `[[clang::address_space(1)]]` spelling
> > and ensure that it is printed properly?
> 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?
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++.
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
leonardchan wrote:
> aaron.ballman wrote:
> > 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?
> 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.
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 177587.
leonardchan marked 3 inline comments as done.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/AST/address_space_attribute.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -128,7 +128,8 @@
if (TU && !T.isNull()) {
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
- if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
}
}
Index: clang/test/AST/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/AST/address_space_attribute.cpp
@@ -0,0 +1,20 @@
+// 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
+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;
+
+ __attribute__((address_space(I))) int *y;
+ // CHECK: VarDecl {{.*}} y '__attribute__((address_space(2))) int *'
+}
+
+void func2() {
+ func<2>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5740,28 +5740,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5770,14 +5769,28 @@
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(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 , 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
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan added inline comments.
Comment at: clang/test/Sema/address_space_attribute.cpp:9
+ // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *'
+ __attribute__((address_space(1))) int *x;
+
aaron.ballman wrote:
> Can you also add a test using the `[[clang::address_space(1)]]` spelling and
> ensure that it is printed properly?
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?
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
aaron.ballman wrote:
> 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?
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman added inline comments.
Comment at: clang/test/Sema/address_space_attribute.cpp:1
+// RUN: %clang_cc1 %s -ast-dump | FileCheck %s
+
This test should be moved to the AST directory instead of Sema.
Comment at: clang/test/Sema/address_space_attribute.cpp:9
+ // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *'
+ __attribute__((address_space(1))) int *x;
+
Can you also add a test using the `[[clang::address_space(1)]]` spelling and
ensure that it is printed properly?
Comment at: clang/tools/libclang/CXType.cpp:132
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
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?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan updated this revision to Diff 177561.
leonardchan marked an inline comment as done.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/test/Sema/address_space_attribute.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -128,7 +128,8 @@
if (TU && !T.isNull()) {
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
- if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
}
}
Index: clang/test/Sema/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/Sema/address_space_attribute.cpp
@@ -0,0 +1,20 @@
+// 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
+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;
+
+ __attribute__((address_space(I))) int *y;
+ // CHECK: VarDecl {{.*}} y '__attribute__((address_space(2))) int *'
+}
+
+void func2() {
+ func<2>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5740,28 +5740,27 @@
// 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 , 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 , LangAS ,
+ const Expr *AddrSpace,
+ SourceLocation AttrLoc) {
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);
}
@@ -5770,14 +5769,28 @@
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(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 , 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
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Missing test cases.
Comment at: clang/lib/Sema/SemaType.cpp:5747
+static bool BuildAddressSpaceIndex(LangAS , const Expr *AddrSpace,
+ SourceLocation AttrLoc, Sema ) {
if (!AddrSpace->isValueDependent()) {
I think we usually pass the `Sema` argument first in these helper methods.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55447/new/
https://reviews.llvm.org/D55447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType
leonardchan created this revision.
leonardchan added reviewers: rsmith, aaron.ballman.
leonardchan added a project: clang.
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.
Repository:
rC Clang
https://reviews.llvm.org/D55447
Files:
clang/include/clang/Sema/Sema.h
clang/lib/AST/TypePrinter.cpp
clang/lib/Sema/SemaType.cpp
clang/tools/libclang/CXType.cpp
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -128,7 +128,8 @@
if (TU && !T.isNull()) {
// Handle attributed types as the original type
if (auto *ATT = T->getAs()) {
- if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
+ if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+ ATT->getAttrKind() != attr::AddressSpace) {
return MakeCXType(ATT->getModifiedType(), TU);
}
}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5740,28 +5740,26 @@
// 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 , 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(LangAS , const Expr *AddrSpace,
+ SourceLocation AttrLoc, Sema ) {
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);
}
@@ -5770,14 +5768,28 @@
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(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 , 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
@@ -5808,6 +5820,14 @@
return Context.getDependentAddressSpaceType(T, AddrSpace, AttrLoc);
}
+QualType Sema::BuildAddressSpaceAttr(QualType , Expr *AddrSpace,
+ SourceLocation AttrLoc) {
+ LangAS ASIdx;
+ if (!BuildAddressSpaceIndex(ASIdx, AddrSpace, AttrLoc, *this))
+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
