[Clang][Sema] properly remove qualifiers in __is_pointer_interconvertible_base_of #167881

rosefromthedead · 2025-11-13T13:56:05Z

getCanonicalTypeUnqualified() doesn't actually remove cv qualifiers if the type is a SubstTemplateTypeParmType. Use getUnqualifiedType() instead, which does.

…ible_base_of getCanonicalTypeUnqualified() doesn't actually remove cv qualifiers if the type is a SubstTemplateTypeParmType. Use getUnqualifiedType() instead, which does. Fixes: llvm#135273

github-actions · 2025-11-13T13:56:25Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-11-13T13:56:58Z

@llvm/pr-subscribers-clang

Author: Rose Hudson (rosefromthedead)

Changes

getCanonicalTypeUnqualified() doesn't actually remove cv qualifiers if the type is a SubstTemplateTypeParmType. Use getUnqualifiedType() instead, which does.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaChecking.cpp (+2-2)
(added) clang/test/Sema/pointer-interconvertible-template-param.cpp (+22)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index a8e3fe6c07b12..73473f1878ed1 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -15607,8 +15607,8 @@ bool Sema::IsLayoutCompatible(QualType T1, QualType T2) const {
 
 bool Sema::IsPointerInterconvertibleBaseOf(const TypeSourceInfo *Base,
                                            const TypeSourceInfo *Derived) {
-  QualType BaseT = Base->getType()->getCanonicalTypeUnqualified();
-  QualType DerivedT = Derived->getType()->getCanonicalTypeUnqualified();
+  QualType BaseT = Base->getType().getUnqualifiedType();
+  QualType DerivedT = Derived->getType().getUnqualifiedType();
 
   if (BaseT->isStructureOrClassType() && DerivedT->isStructureOrClassType() &&
       getASTContext().hasSameType(BaseT, DerivedT))
diff --git a/clang/test/Sema/pointer-interconvertible-template-param.cpp b/clang/test/Sema/pointer-interconvertible-template-param.cpp
new file mode 100644
index 0000000000000..174ac9a44a639
--- /dev/null
+++ b/clang/test/Sema/pointer-interconvertible-template-param.cpp
@@ -0,0 +1,22 @@
+// RUN: %clangxx %s -o %t
+// RUN: %t | FileCheck %s
+
+#include <iostream>
+
+class A {};
+class B : public A {};
+
+template <class _Base, class _Derived>
+inline constexpr bool is_pointer_interconvertible_base_of_v = __is_pointer_interconvertible_base_of(_Base, _Derived);
+
+int main() {
+    // CHECK: 1
+    std::cout << __is_pointer_interconvertible_base_of(const A, A) << std::endl;
+    // CHECK-NEXT: 1
+    std::cout << is_pointer_interconvertible_base_of_v<const A, A> << std::endl;
+
+    // CHECK-NEXT: 1
+    std::cout << __is_pointer_interconvertible_base_of(const A, B) << std::endl;
+    // CHECK-NEXT: 1
+    std::cout << is_pointer_interconvertible_base_of_v<const A, B> << std::endl;
+}

AaronBallman

Thank you for the fix! The changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the fix. But also, do we need to put it under the potentially breaking changes section because now template specializations may silently change behavior because the type trait now returns a different value in some situations? (Do we think the breakage needs some ABI versioning so users can get the old behavior if they were relying on it?)

zwuis · 2025-11-13T14:30:42Z

clang/test/Sema/pointer-interconvertible-template-param.cpp

It would be better to put these tests in "clang/test/SemaCXX/type-traits.cpp".

erichkeane · 2025-11-13T14:35:58Z

clang/lib/Sema/SemaChecking.cpp

-  QualType BaseT = Base->getType()->getCanonicalTypeUnqualified();
-  QualType DerivedT = Derived->getType()->getCanonicalTypeUnqualified();
+  QualType BaseT = Base->getType().getUnqualifiedType();
+  QualType DerivedT = Derived->getType().getUnqualifiedType();


Can you better explain why this is the right answer here? And why loss of canonicalization didn't affect anything?

I misread the def of getUnqualifiedType() at first, now I'm not sure this is the correct solution. I thought it did canonicalise, but actually it desugars, which I don't know the intention behind but it seems to do near enough the same thing as canonicalisation that the tests passed.

Is getCanonicalTypeUnqualified() supposed to behave as it does? It returns the canonical type directly without explicitly stripping quals, but in the case of SubstTemplateTypeParmType the canonical type can have quals (and there are tests that rely on this).

I'm a little surprised getCanonicalTypeUnqualified actually resulted in qualifiers. Qualifiers are stored in the QualType, which gets stripped off by the operator ->. Canonical Unqualified should be exactly the unqualified type after canonicalization, so I'm a little shocked you're not seeing it work.

Hmm, instead of this change, the following also passes the tests:

diff --git a/clang/include/clang/AST/CanonicalType.h b/clang/include/clang/AST/CanonicalType.h index 87bbd7b5d885..c1d590b79792 100644 --- a/clang/include/clang/AST/CanonicalType.h +++ b/clang/include/clang/AST/CanonicalType.h @@ -213,7 +213,7 @@ inline bool operator!=(CanQual<T> x, CanQual<U> y) { using CanQualType = CanQual<Type>; inline CanQualType Type::getCanonicalTypeUnqualified() const { - return CanQualType::CreateUnsafe(getCanonicalTypeInternal()); + return CanQualType::CreateUnsafe(getCanonicalTypeInternal().getUnqualifiedType()); } inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

It's surprising to me that we never ran into this before, but maybe this patch is more reasonable/correct.

Hrmph... I would expect the canonical type to be without qualifiers, so that extra getUnqualifiedType shouldn't be necessary. SOMEHOW we're failing to correctly canonicalize when creating the type? See isCanonicalUnqualified.

EDIT: NOPE, that actually isn't right, the isCanonicalUnqualified is asking a different question :) I THINK your proposed change there is perhaps right, I'd like to have another set or three of eyes on it.

@AaronBallman @cor3ntin @mizvekov ?

also, if canonical types can have qualifiers, why is A the canonical type of const A?

also, if canonical types can have qualifiers, why is A the canonical type of const A?

I'm not certain why template type parameter substitution types can have canonical types with qualifiers. Maybe @erichkeane @mizvekov @zygoloid or someone else knows?

Well, we need references at least (since thats how reference folding needs to work/move semantics/etc). So I would presume that cv qualifiers would be the same way. So we'd need to maintain those.

A canonical type being qualified isn't surprising to me -- that happens frequently (eg, the canonical type of a typedef can be qualified).

I'm not sure what the intent of getCanonicalTypeUnqualified is, but it looks very strange. If it's supposed to be returning an unqualified type, why isn't it removing the qualifiers and returning a const Type*? Maybe the intent is to provide "get canonical type and I don't care about whether you preserve all, some, or none of the qualifiers", but the name really doesn't convey that.

Looking through the calls, it seems most fall into one of three categories:

getCanonicalTypeUnqualified().getUnqualifiedType() -- actually discard the qualifiers and produce an unqualified QualType or CanQualType

getCanonicalTypeUnqualified()->something or getCanonicalTypeUnqualified().getTypePtr() -- actually discard the qualifiers and produce a const Type*

Calls that look suspicious and are probably wrong

:) It'd probably be good to remove this hazard, make the function actually return an unqualified type, and check all the callers to make sure none of them really cares about the qualifiers (one would hope they don't!).

(But in passing, maybe some or most of the callers should just be using getUnqualifiedType rather than removing type sugar, and we could make them do so?)

(But in passing, maybe some or most of the callers should just be using getUnqualifiedType rather than removing type sugar, and we could make them do so?)

Well, if we're going to be poking at those calls, we also should be checking whether they should be using getAtomicUnqualifiedType() thanks to _Atomic in C being such a weird beast; we've had numerous bugs where we accidentally fail to strip atomic qualifiers and that causes further issues.

zygoloid · 2025-11-17T19:08:06Z

clang/test/Sema/pointer-interconvertible-template-param.cpp

@@ -0,0 +1,22 @@
+// RUN: %clangxx %s -o %t
+// RUN: %t | FileCheck %s


We don't use execution tests. You can test this with static_assert instead.

zygoloid · 2025-11-17T19:17:04Z

clang/lib/Sema/SemaChecking.cpp

-  QualType BaseT = Base->getType()->getCanonicalTypeUnqualified();
-  QualType DerivedT = Derived->getType()->getCanonicalTypeUnqualified();
+  QualType BaseT = Base->getType().getUnqualifiedType();
+  QualType DerivedT = Derived->getType().getUnqualifiedType();


A canonical type being qualified isn't surprising to me -- that happens frequently (eg, the canonical type of a typedef can be qualified).

I'm not sure what the intent of getCanonicalTypeUnqualified is, but it looks very strange. If it's supposed to be returning an unqualified type, why isn't it removing the qualifiers and returning a const Type*? Maybe the intent is to provide "get canonical type and I don't care about whether you preserve all, some, or none of the qualifiers", but the name really doesn't convey that.

Looking through the calls, it seems most fall into one of three categories:

getCanonicalTypeUnqualified().getUnqualifiedType() -- actually discard the qualifiers and produce an unqualified QualType or CanQualType

getCanonicalTypeUnqualified()->something or getCanonicalTypeUnqualified().getTypePtr() -- actually discard the qualifiers and produce a const Type*

Calls that look suspicious and are probably wrong

:) It'd probably be good to remove this hazard, make the function actually return an unqualified type, and check all the callers to make sure none of them really cares about the qualifiers (one would hope they don't!).

(But in passing, maybe some or most of the callers should just be using getUnqualifiedType rather than removing type sugar, and we could make them do so?)

[Clang][Sema] properly remove qualifiers in __is_pointer_interconvert…

0627f97

…ible_base_of getCanonicalTypeUnqualified() doesn't actually remove cv qualifiers if the type is a SubstTemplateTypeParmType. Use getUnqualifiedType() instead, which does. Fixes: llvm#135273

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 13, 2025

AaronBallman requested review from cor3ntin, erichkeane and philnik777 November 13, 2025 14:19

AaronBallman reviewed Nov 13, 2025

View reviewed changes

zwuis reviewed Nov 13, 2025

View reviewed changes

erichkeane reviewed Nov 13, 2025

View reviewed changes

zygoloid reviewed Nov 17, 2025

View reviewed changes

		@@ -0,0 +1,22 @@
		// RUN: %clangxx %s -o %t
		// RUN: %t \| FileCheck %s

[Clang][Sema] properly remove qualifiers in __is_pointer_interconvertible_base_of #167881

Are you sure you want to change the base?

[Clang][Sema] properly remove qualifiers in __is_pointer_interconvertible_base_of #167881

Conversation

rosefromthedead commented Nov 13, 2025

Uh oh!

github-actions bot commented Nov 13, 2025

Uh oh!

llvmbot commented Nov 13, 2025

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erichkeane Nov 13, 2025 •

edited

Loading