Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+24 −2
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Collaborator

@erichkeane erichkeane Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@rosefromthedead rosefromthedead Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@erichkeane erichkeane Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@rosefromthedead rosefromthedead Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@erichkeane erichkeane Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

@rosefromthedead rosefromthedead Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@AaronBallman AaronBallman Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@erichkeane erichkeane Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@zygoloid zygoloid Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. getCanonicalTypeUnqualified().getUnqualifiedType() -- actually discard the qualifiers and produce an unqualified QualType or CanQualType
  2. getCanonicalTypeUnqualified()->something or getCanonicalTypeUnqualified().getTypePtr() -- actually discard the qualifiers and produce a const Type*
  3. 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?)

Copy link
Collaborator

@AaronBallman AaronBallman Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.


if (BaseT->isStructureOrClassType() && DerivedT->isStructureOrClassType() &&
getASTContext().hasSameType(BaseT, DerivedT))
Expand Down
22 changes: 22 additions & 0 deletions clang/test/Sema/pointer-interconvertible-template-param.cpp
View file
Open in desktop
Copy link
Contributor

@zwuis zwuis Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// RUN: %clangxx %s -o %t
// RUN: %t | FileCheck %s
Copy link
Collaborator

@zygoloid zygoloid Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#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;
}
Loading