[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I've done a pass through this file looking for places where we incorrectly add to the ODR hash a type that was written within some other entity than the one that we're ODR hashing, that could validly be spelled differently in different declarations of that other entity. There are quite a lot of them; please see the comments here for places that need fixing.

Ideally, we should only be hashing a type when we have a corresponding `TypeSourceInfo` (that describes how that type was written in the source code) and hence a `TypeLoc`. Similarly, for template arguments, we'd like to have a `TemplateArguentLoc` instead of a `TemplateArgument`. So if you want to handle this properly, the best thing would be to change this code so that it can only hash `TemplateArgumentLoc`s and `TypeLoc`s, not `TemplateArgument`s and `QualType`s, but that would be a substantial amount of work; just changing it so we start with a type-as-written should be good enough to get it working properly.



================
Comment at: clang/lib/AST/ODRHash.cpp:297-302
   void VisitValueDecl(const ValueDecl *D) {
     if (!isa<FunctionDecl>(D)) {
       AddQualType(D->getType());
     }
     Inherited::VisitValueDecl(D);
   }
----------------
For a `DeclaratorDecl` we should be adding `D->getTypeSourceInfo()->getType()` (the type as written), not `D->getType()` (the resolved type); for a `ValueDecl` that is not a `DeclaratorDecl`, we shouldn't include the type at all, because it wasn't written in the source code.


================
Comment at: clang/lib/AST/ODRHash.cpp:354
     ID.AddInteger(D->getPropertyImplementation());
     AddQualType(D->getType());
     AddDecl(D);
----------------



================
Comment at: clang/lib/AST/ODRHash.cpp:397
 
     AddQualType(Method->getReturnType());
     ID.AddInteger(Method->param_size());
----------------



================
Comment at: clang/lib/AST/ODRHash.cpp:862-910
   // Return the RecordType if the typedef only strips away a keyword.
   // Otherwise, return the original type.
   static const Type *RemoveTypedef(const Type *T) {
     const auto *TypedefT = dyn_cast<TypedefType>(T);
     if (!TypedefT) {
       return T;
     }
----------------
We should not be stripping typedefs here!


================
Comment at: clang/lib/AST/ODRHash.cpp:915-939
     QualType Original = T->getOriginalType();
     QualType Adjusted = T->getAdjustedType();
 
     // The original type and pointee type can be the same, as in the case of
     // function pointers decaying to themselves.  Set a bool and only process
     // the type once, to prevent doubling the work.
     SplitQualType split = Adjusted.split();
----------------
We should only be hashing the type that was written in the source code here, not the adjusted type that's computed from it (and might partially desugar that original type).


================
Comment at: clang/lib/AST/ODRHash.cpp:976
     AddQualType(T->getModifiedType());
     AddQualType(T->getEquivalentType());
 
----------------
We should defensively not include the equivalent type here, because it wasn't written in the entity that we're ODR hashing and might in principle depend on the spelling of a type elsewhere (depending on what the attribute does). The modified type is written in the source so it's fine to include it.


================
Comment at: clang/lib/AST/ODRHash.cpp:998
     AddStmt(T->getUnderlyingExpr());
     AddQualType(T->getUnderlyingType());
     VisitType(T);
----------------
We should not hash this, because it can differ between identical types that are written the same way.


================
Comment at: clang/lib/AST/ODRHash.cpp:1187-1203
     QualType UnderlyingType = T->getDecl()->getUnderlyingType();
     VisitQualifiers(UnderlyingType.getQualifiers());
     while (true) {
       if (const TypedefType *Underlying =
               dyn_cast<TypedefType>(UnderlyingType.getTypePtr())) {
         UnderlyingType = Underlying->getDecl()->getUnderlyingType();
         continue;
----------------
This code is also wrong, and looks like the root cause of the issue here. We shouldn't be including the underlying type of a typedef type in the ODR hash, because it can be written differently in different declarations of the typedef declaration.

If we want to include the underlying type of the typedef here, we'll need a new kind of hashing to capture only the value of the typedef and not how it was written. I don't think it's worth it; let's just not include the definition of a referenced typedef in the hash for now.


================
Comment at: clang/lib/AST/ODRHash.cpp:1210-1211
     Hash.AddBoolean(T->isSugared());
     if (T->isSugared())
       AddQualType(T->desugar());
 
----------------
We should also not hash this, because it can differ between identical types.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154324/new/

https://reviews.llvm.org/D154324