[ASTContext] Make getAddrSpaceQualType replace address spaces.

[ebevhan]

Summary

The documentation for getAddrSpaceQualType says: "If T already
has an address space specifier, it is silently replaced."

The function did not do this; it asserted instead. Fix it so
that it behaves according to the comment.

[rjmccall]

Is there a specific reason to change the implementation instead of changing the documentation?

[ebevhan]

I have this patch uploaded as well: https://reviews.llvm.org/D47630

I suspected that there might be cases for which we would try adding the same address space to a type, but I noticed that this method doesn't replace properly.
I don't actually have an example of this, though. It's possible that it will work anyway.

[rjmccall]

Well, Sema should always be diagnosing conflicts.

If you don't have a specific reason to allow replacement, I would prefer just fixing the documentation to state this as a precondition.

[ebevhan]

There's actually a bit more to it than in these two patches. The complete diff to this function in our downstream Clang looks like this:

 QualType
 ASTContext::getAddrSpaceQualType(QualType T, unsigned AddressSpace) const {
-  QualType CanT = getCanonicalType(T);
-  if (CanT.getAddressSpace() == AddressSpace)
+  if (T.getLocalQualifiers().getAddressSpace() == AddressSpace)
     return T;
 
   // If we are composing extended qualifiers together, merge together
   // into one ExtQuals node.
   QualifierCollector Quals;
   const Type *TypeNode = Quals.strip(T);
 
   // If this type already has an address space specified, it cannot get
   // another one.
-  assert(!Quals.hasAddressSpace() &&
-         "Type cannot be in multiple addr spaces!");
-  Quals.addAddressSpace(AddressSpace);
+  if (Quals.hasAddressSpace())
+    Quals.removeAddressSpace();
+  if (AddressSpace)
+    Quals.addAddressSpace(AddressSpace);
 
   return getExtQualType(TypeNode, Quals);
 }

In our downstream Clang, functions have address spaces. The desired address space is applied in getFunctionType, using getAddrSpaceQualType. Due to how FunctionTypes are built, it's possible to end up with noncanonical FunctionType that doesn't have an address space whose canonical type does have one. For example, when building the noncanonical type void (*)(void * const), we will first build its canonical type void (_AS *)(void *). Since getAddrSpaceQualType looks at the canonical type to determine whether the address space is already applied, it's skipped and we end up with this discrepancy.

Now that I test it again, I can't seem to induce the assertion. I suspect I might just have changed it because the documentation said that was how it was supposed to work. Perhaps I should be submitting the upper diff instead? Though, considering that this cannot be reproduced in trunk maybe I simply shouldn't submit it at all.

[rjmccall]

In D47627#1121970, @ebevhan wrote:

There's actually a bit more to it than in these two patches. The complete diff to this function in our downstream Clang looks like this:

 QualType
 ASTContext::getAddrSpaceQualType(QualType T, unsigned AddressSpace) const {
-  QualType CanT = getCanonicalType(T);
-  if (CanT.getAddressSpace() == AddressSpace)
+  if (T.getLocalQualifiers().getAddressSpace() == AddressSpace)
     return T;
 
   // If we are composing extended qualifiers together, merge together
   // into one ExtQuals node.
   QualifierCollector Quals;
   const Type *TypeNode = Quals.strip(T);
 
   // If this type already has an address space specified, it cannot get
   // another one.
-  assert(!Quals.hasAddressSpace() &&
-         "Type cannot be in multiple addr spaces!");
-  Quals.addAddressSpace(AddressSpace);
+  if (Quals.hasAddressSpace())
+    Quals.removeAddressSpace();
+  if (AddressSpace)
+    Quals.addAddressSpace(AddressSpace);
 
   return getExtQualType(TypeNode, Quals);
 }

In our downstream Clang, functions have address spaces. The desired address space is applied in getFunctionType, using getAddrSpaceQualType. Due to how FunctionTypes are built, it's possible to end up with noncanonical FunctionType that doesn't have an address space whose canonical type does have one. For example, when building the noncanonical type void (*)(void * const), we will first build its canonical type void (_AS *)(void *). Since getAddrSpaceQualType looks at the canonical type to determine whether the address space is already applied, it's skipped and we end up with this discrepancy.

Now that I test it again, I can't seem to induce the assertion. I suspect I might just have changed it because the documentation said that was how it was supposed to work. Perhaps I should be submitting the upper diff instead? Though, considering that this cannot be reproduced in trunk maybe I simply shouldn't submit it at all.

Well, the documentation mismatch is worth fixing even if the code isn't. But I think at best your use-case calls for weakening the assertion to be that any existing address space isn't *different*, yeah.

Separately, I'm not sure that's really the right representation for a Harvard architecture (which is what I assume you're trying to extend Clang to support); I think you should probably just teach the compiler that function pointers are different.

[ebevhan]

Well, the documentation mismatch is worth fixing even if the code isn't. But I think at best your use-case calls for weakening the assertion to be that any existing address space isn't *different*, yeah.

Alright, I'll give that a shot.

Separately, I'm not sure that's really the right representation for a Harvard architecture (which is what I assume you're trying to extend Clang to support); I think you should probably just teach the compiler that function pointers are different.

Well, we've already implemented it and it's been running in our downstream for a while without issues at this point. We just figured it was less work to use the existing address space support for it than to hack special cases all over the place for functions and function pointers.

[arichardson]

Well, the documentation mismatch is worth fixing even if the code isn't. But I think at best your use-case calls for weakening the assertion to be that any existing address space isn't *different*, yeah.

Separately, I'm not sure that's really the right representation for a Harvard architecture (which is what I assume you're trying to extend Clang to support); I think you should probably just teach the compiler that function pointers are different.

We have similar issues with CHERI and for us representing it at the clang AST level doesn't work well. What we really need is address spaces on the LLVM function: For that I've submitted is D47541 (based on D37054). I wonder if that would be a better match for your problem? We put all functions either into LLVM address spac 0 or 200 (depending on the flags passed to clang) so we don't need to add any additional information to the clang AST. Do you have different address spaces for different functions or are they just different from data?

[rjmccall]

In D47627#1127716, @ebevhan wrote:

Well, the documentation mismatch is worth fixing even if the code isn't. But I think at best your use-case calls for weakening the assertion to be that any existing address space isn't *different*, yeah.

Alright, I'll give that a shot.

Separately, I'm not sure that's really the right representation for a Harvard architecture (which is what I assume you're trying to extend Clang to support); I think you should probably just teach the compiler that function pointers are different.

Well, we've already implemented it and it's been running in our downstream for a while without issues at this point. We just figured it was less work to use the existing address space support for it than to hack special cases all over the place for functions and function pointers.

I'm going to insist that you try it before you can upstream, I'm afraid.

lib/AST/ASTContext.cpp

if (CanT.getAddressSpace() == AddressSpace)
     return T;
 
   // If we are composing extended qualifiers together, merge together
   // into one ExtQuals node.
   QualifierCollector Quals;
   const Type *TypeNode = Quals.strip(T);
 
   // If this type already has an address space specified, it cannot get
-  // another one.
-  assert(!Quals.hasAddressSpace() &&
-         "Type cannot be in multiple addr spaces!");
+  // another one. Replace it.
+  if (Quals.hasAddressSpace())
+    Quals.removeAddressSpace();
+  if (AddressSpace != LangAS::Default)
     Quals.addAddressSpace(AddressSpace);
 
+  if (Quals.hasNonFastQualifiers())
     return getExtQualType(TypeNode, Quals);
+  else
+    return QualType(TypeNode, Quals.getFastQualifiers());
 }
 
 QualType ASTContext::removeAddrSpaceQualType(QualType T) const {
   // If we are composing extended qualifiers together, merge together
   // into one ExtQuals node.
   QualifierCollector Quals;
   const Type *TypeNode = Quals.strip(T);