[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D47627