[clang] Make sure the same UsingType is searched and inserted

[apolloww]

When creating a new UsingType, the underlying type may change if it is a declaration. This creates an inconsistency between the type searched and type created. Update member and non-member Profile functions so that they return the same ID.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Wei Wang (apolloww)

Changes

When creating a new UsingType, the underlying type may change if it is a declaration. This creates an inconsistency between the type searched and type created. Move the update before search so that we always use the same type for search and creation.

---

Full diff: https://github.com/llvm/llvm-project/pull/79182.diff

1 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+7-6)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 5eb7aa3664569dd..d312ef61fb15d4a 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -4672,12 +4672,6 @@ QualType ASTContext::getTypedefType(const TypedefNameDecl *Decl,
 QualType ASTContext::getUsingType(const UsingShadowDecl *Found,
                                   QualType Underlying) const {
   llvm::FoldingSetNodeID ID;
-  UsingType::Profile(ID, Found, Underlying);
-
-  void *InsertPos = nullptr;
-  if (UsingType *T = UsingTypes.FindNodeOrInsertPos(ID, InsertPos))
-    return QualType(T, 0);
-
   const Type *TypeForDecl =
       cast<TypeDecl>(Found->getTargetDecl())->getTypeForDecl();
 
@@ -4687,6 +4681,13 @@ QualType ASTContext::getUsingType(const UsingShadowDecl *Found,
 
   if (Underlying.getTypePtr() == TypeForDecl)
     Underlying = QualType();
+  UsingType::Profile(ID, Found, Underlying);
+
+  void *InsertPos = nullptr;
+  if (UsingType *T = UsingTypes.FindNodeOrInsertPos(ID, InsertPos)) {
+    return QualType(T, 0);
+  }
+
   void *Mem =
       Allocate(UsingType::totalSizeToAlloc<QualType>(!Underlying.isNull()),
                alignof(UsingType));

[apolloww]

We saw a huge build speed regression from internal codebase when migrating to clang-17 triggered by this issue. The search for the same UsingType always ends up with "not found" and the folding set UsingTypes contains lots of duplicated notes.

This is a quick fix I come up with. Feel free to propose a better one because I don't work in clang front end often. If it looks good, I'll try to add a test case.

[mizvekov]

The change is correct. The problem is subtle though. It comes from the difference in behavior between the member and non-member Profile functions.

I think we could do better instead with a change which makes it harder to trip on this.

I think a simplification like this should work (untested):

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index ea425791fc97..3d411051084c 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -4729,13 +4729,12 @@ public:
   bool typeMatchesDecl() const { return !UsingBits.hasTypeDifferentFromDecl; }

   void Profile(llvm::FoldingSetNodeID &ID) {
-    Profile(ID, Found, typeMatchesDecl() ? QualType() : getUnderlyingType());
+    Profile(ID, Found, getUnderlyingType());
   }
   static void Profile(llvm::FoldingSetNodeID &ID, const UsingShadowDecl *Found,
                       QualType Underlying) {
     ID.AddPointer(Found);
-    if (!Underlying.isNull())
-      Underlying.Profile(ID);
+    Underlying.Profile(ID);
   }
   static bool classof(const Type *T) { return T->getTypeClass() == Using; }
 };

[apolloww]

The change is correct. The problem is subtle though. It comes from the difference in behavior between the member and non-member Profile functions.

I think we could do better instead with a change which makes it harder to trip on this.

I think a simplification like this should work (untested):

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index ea425791fc97..3d411051084c 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -4729,13 +4729,12 @@ public:
   bool typeMatchesDecl() const { return !UsingBits.hasTypeDifferentFromDecl; }

   void Profile(llvm::FoldingSetNodeID &ID) {
-    Profile(ID, Found, typeMatchesDecl() ? QualType() : getUnderlyingType());
+    Profile(ID, Found, getUnderlyingType());
   }
   static void Profile(llvm::FoldingSetNodeID &ID, const UsingShadowDecl *Found,
                       QualType Underlying) {
     ID.AddPointer(Found);
-    if (!Underlying.isNull())
-      Underlying.Profile(ID);
+    Underlying.Profile(ID);
   }
   static bool classof(const Type *T) { return T->getTypeClass() == Using; }
 };

Thanks. This works and looks better.

Wonder what test should I add here. This only affects the content of the internal folding set so we always create new types but result is still correct.

[mizvekov]

Wonder what test should I add here. This only affects the content of the internal folding set so we always create new types but result is still correct.

One possibility is doing an AST test, there are some examples in there that show how you can test two different AST nodes have the same address.

The best here would be some kind of performance regression test. We don't have anything like that per-se in the test suite unfortunately.
But if you can isolate a good test case, you can make it a normal regression test, which with the fix is still within CI limits of time and space, but which pushes much farther out without the fix. This would not be without precedent, there already exists tests like that.

[apolloww]

Thanks. Added test case.

[mizvekov]

LGTM.

Thanks!