[Sema] Iteratively strip sugar when removing address spaces.

[ebevhan]

Summary

ASTContext::removeAddrSpaceQualType does not properly deal
with sugar. QualTypes derive their ASes from the AS on the
canonical type, not the type itself. However,
removeAddrSpaceQualType only strips the outermost qualifiers,
which means that it can fail to remove addrspace qualifiers
if there is sugar in the way.

Change the function to desugar types until the address space
really no longer exists on the corresponding QualType. This
should guarantee the removal of the address space.

This fixes the erroneous behavior in D62574.

[svenvh]

Wondering if stripping the sugar is the right thing to do here, because it means we don't have any sugar on the resulting type if it has gone through the SCS and pointer type reconstruction?

I'm unsure if this is currently broken upstream without other patches to exploit the behavior,

I'm not sure either; perhaps @rjmccall has some insights.

[rjmccall]

removeAddrSpaceQualType should guarantee that it removes the address space qualifier; you shouldn't need to do something special here. That means it needs to iteratively desugar and collect qualifiers as long as the type is still address-space-qualified.

[ebevhan]

In D83325#2170813, @rjmccall wrote:

removeAddrSpaceQualType should guarantee that it removes the address space qualifier; you shouldn't need to do something special here. That means it needs to iteratively desugar and collect qualifiers as long as the type is still address-space-qualified.

Well, from what I observed, it is not doing that, so perhaps that's what needs to be addressed. That's a bit more involved, though.

We will still end up stripping all sugar up until we hit the AS-qualification, though. Unless we are supposed to reconstitute the sugared type with the AS altered somehow?

[rjmccall]

There's no way to do that, no. Stripping sugar down to the point where you don't have that qualifier anymore is the best we can do.

[ebevhan]

Redid patch. Now we properly strip ASes from types by desugaring.

[rjmccall]

LGTM

[svenvh]

LGTM, but just wondering if the test actually belongs to this patch, as it doesn't demonstrate the problem without one of your other patches?

[ebevhan]

In D83325#2209589, @svenvh wrote:

LGTM, but just wondering if the test actually belongs to this patch, as it doesn't demonstrate the problem without one of your other patches?

Mm, that is true, but then I'd be submitting the patch without a test and that doesn't seem quite right.

I'm unsure if there's a way to exhibit the failure without the followup patch.

clang/lib/AST/ASTContext.cpp

QualType ASTContext::getAddrSpaceQualType(QualType T,
   assert(!Quals.hasAddressSpace() &&
          "Type cannot be in multiple addr spaces!");
   Quals.addAddressSpace(AddressSpace);
 
   return getExtQualType(TypeNode, Quals);
 }
 
 QualType ASTContext::removeAddrSpaceQualType(QualType T) const {
+  // If the type is not qualified with an address space, just return it
+  // immediately.
+  if (!T.hasAddressSpace())
+    return T;
+
   // If we are composing extended qualifiers together, merge together
   // into one ExtQuals node.
   QualifierCollector Quals;
-  const Type *TypeNode = Quals.strip(T);
+  const Type *TypeNode;
 
-  // If the qualifier doesn't have an address space just return it.
-  if (!Quals.hasAddressSpace())
-    return T;
+  while (T.hasAddressSpace()) {
+    TypeNode = Quals.strip(T);
+
+    // If the type no longer has an address space after stripping qualifiers,
+    // jump out.
+    if (!QualType(TypeNode, 0).hasAddressSpace())
+      break;
+
+    // There might be sugar in the way. Strip it and try again.
+    T = T.getSingleStepDesugaredType(*this);
+  }
 
   Quals.removeAddressSpace();
 
   // Removal of the address space can mean there are no longer any
   // non-fast qualifiers, so creating an ExtQualType isn't possible (asserts)
   // or required.
   if (Quals.hasNonFastQualifiers())
     return getExtQualType(TypeNode, Quals);

clang/test/CodeGenCXX/address-space-cast.cpp

 // RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -emit-llvm -o - | FileCheck %s
 
 #define __private__ __attribute__((address_space(5)))
 
 void func_pchar(__private__ char *x);
 void func_pvoid(__private__ void *x);
 void func_pint(__private__ int *x);
 
+class Base {
+};
+
+class Derived : public Base {
+};
+
+void fn(Derived *p) {
+  __private__ Base *b = (__private__ Base *)p;
+}
+
 void test_cast(char *gen_char_ptr, void *gen_void_ptr, int *gen_int_ptr) {
   // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i8 addrspace(5)*
   // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
   __private__ char *priv_char_ptr = (__private__ char *)gen_char_ptr;
 
   // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i8 addrspace(5)*
   // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
   priv_char_ptr = (__private__ char *)gen_void_ptr;