This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Fix Modified Type in address_space AttributedType
ClosedPublic

Authored by leonardchan on Dec 7 2018, 11:26 AM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

aaron.ballman requested changes to this revision.Dec 10 2018, 5:13 AM

Missing test cases.

clang/lib/Sema/SemaType.cpp
5763

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

This revision now requires changes to proceed.Dec 10 2018, 5:13 AM
leonardchan marked an inline comment as done.
aaron.ballman added inline comments.Dec 10 2018, 12:07 PM
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

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 marked 3 inline comments as done.
leonardchan added inline comments.
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

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 added inline comments.Dec 10 2018, 12:36 PM
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

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 marked 2 inline comments as done.
  • Added the CXX11 test
clang/tools/libclang/CXType.cpp
132

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

aaron.ballman added inline comments.Dec 11 2018, 12:00 PM
clang/tools/libclang/CXType.cpp
132

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 marked an inline comment as done.Dec 11 2018, 2:01 PM
leonardchan added inline comments.
clang/tools/libclang/CXType.cpp
132

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

aaron.ballman added inline comments.Dec 12 2018, 5:02 AM
clang/tools/libclang/CXType.cpp
132

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

leonardchan marked an inline comment as done.Jan 14 2019, 9:45 PM
leonardchan added inline comments.
clang/tools/libclang/CXType.cpp
132

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 added inline comments.Jan 15 2019, 1:00 PM
clang/tools/libclang/CXType.cpp
132

Which test was failing because of this?

leonardchan marked an inline comment as done.Jan 15 2019, 1:05 PM
leonardchan added inline comments.
clang/tools/libclang/CXType.cpp
132

clang/test/Index/print-type.m

leonardchan marked an inline comment as done.Jan 15 2019, 1:09 PM
leonardchan added inline comments.
clang/tools/libclang/CXType.cpp
132

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 added inline comments.Jan 15 2019, 1:14 PM
clang/tools/libclang/CXType.cpp
132

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 marked an inline comment as done.
leonardchan marked an inline comment as done.Jan 15 2019, 3:30 PM
leonardchan added inline comments.
clang/tools/libclang/CXType.cpp
132

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

aaron.ballman added inline comments.Jan 16 2019, 6:04 AM
clang/tools/libclang/CXType.cpp
132

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

leonardchan marked an inline comment as done.
aaron.ballman accepted this revision.Jan 17 2019, 10:51 AM

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.

This revision is now accepted and ready to land.Jan 17 2019, 10:51 AM

@rsmith Any comments on this patch before submitting?

This revision was automatically updated to reflect the committed changes.