[Clang] remove qualifiers in getCanonicalTypeUnqualified

[rosefromthedead]

It was assumed that since this is a method on Type, there are no quals present because usually you get a Type by using operator-> on a QualType. However, quals aren't always stored in the outermost QualType. For example, if const A is used as a template parameter, the outer QualType does not have the const flag set. The const actually lives in the canonical type inside a SubstTemplateTypeParmType. We therefore need to explicitly remove quals before returning.

Fixes #135273

---

I've been looking at all the call sites as suggested in #167881 (comment) to check that their usage of this method is sane. So far, most of the sites seem to either want unqualified types or not care, so this patch should handle some spooky edge cases of those. Some of them need more thinking, which I'll do soon.

Supersedes #167881

[github-actions]

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.

[rosefromthedead]

cc @erichkeane @AaronBallman @zygoloid

[cor3ntin]

This change needs a release note.
Please add an entry to clang/docs/ReleaseNotes.rst in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!

[llvmbot]

@llvm/pr-subscribers-clang

Author: Rose Hudson (rosefromthedead)

Changes

It was assumed that since this is a method on Type, there are no quals present because usually you get a Type by using operator-> on a QualType. However, quals aren't always stored in the outermost QualType. For example, if const A is used as a template parameter, the outer QualType does not have the const flag set. The const actually lives in the canonical type inside a SubstTemplateTypeParmType. We therefore need to explicitly remove quals before returning.

Fixes #135273

---

I've been looking at all the call sites as suggested in #167881 (comment) to check that their usage of this method is sane. So far, most of the sites seem to either want unqualified types or not care, so this patch should handle some spooky edge cases of those. Some of them need more thinking, which I'll do soon.

Supersedes #167881

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/include/clang/AST/CanonicalType.h (+2-1)
• (modified) clang/test/SemaCXX/type-traits.cpp (+10)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index da064534c25d9..0aa899817f3ea 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -567,6 +567,7 @@ Bug Fixes to C++ Support - Fix a crash when extracting unavailable member type from alias in template deduction. (#GH165560) - Fix incorrect diagnostics for lambdas with init-captures inside braced initializers. (#GH163498) - Fixed spurious diagnoses of certain nested lambda expressions. (#GH149121) (#GH156579) +- Fix the result of ``__is_pointer_interconvertible_base_of`` when arguments are qualified and passed via template parameters. (#GH135273) Bug Fixes to AST Handling ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/AST/CanonicalType.h b/clang/include/clang/AST/CanonicalType.h index 87bbd7b5d885d..b46fc8f50fa23 100644 --- a/clang/include/clang/AST/CanonicalType.h +++ b/clang/include/clang/AST/CanonicalType.h @@ -213,7 +213,8 @@ 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, diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp index 9ef44d0346b48..561c9ca8286b9 100644 --- a/clang/test/SemaCXX/type-traits.cpp +++ b/clang/test/SemaCXX/type-traits.cpp @@ -1812,6 +1812,11 @@ union Union {}; union UnionIncomplete; struct StructIncomplete; // #StructIncomplete +#if __cplusplus >= 201402L +template<class _Base, class _Derived> +inline constexpr bool IsPointerInterconvertibleBaseOfV = __is_pointer_interconvertible_base_of(_Base, _Derived); +#endif + void is_pointer_interconvertible_base_of(int n) { static_assert(__is_pointer_interconvertible_base_of(Base, Derived)); @@ -1880,6 +1885,11 @@ void is_pointer_interconvertible_base_of(int n) static_assert(!__is_pointer_interconvertible_base_of(void(&)(int), void(&)(char))); static_assert(!__is_pointer_interconvertible_base_of(void(*)(int), void(*)(int))); static_assert(!__is_pointer_interconvertible_base_of(void(*)(int), void(*)(char))); + +#if __cplusplus >= 201402L + static_assert(IsPointerInterconvertibleBaseOfV<const Base, Base>); + static_assert(IsPointerInterconvertibleBaseOfV<const Base, Derived>); +#endif } } 

[mizvekov]

LGTM, Thanks.

[AaronBallman]

The changes LGTM but I do have a question: any time we change how type traits are resolved, we silently break ABI for some users. Should we have an ABI versioning check so people can get the old behavior back if they need to? I don't think __is_pointer_interconvertible_base_of() is used so often that we need to (https://sourcegraph.com/search?q=context:global+lang:C%2B%2B+__is_pointer_interconvertible_base_of+count:10000000+-file:.*clang.*+-file:.*test.*&patternType=keyword&sm=0) but I still think the question is worth asking others.

[cor3ntin]

@AaronBallman right, it's a conformance fix to a recently added built-in. I would hope no one relies on the old behavior nor should we keep the old behavior around.

[AaronBallman]

@AaronBallman right, it's a conformance fix to a recently added built-in. I would hope no one relies on the old behavior nor should we keep the old behavior around.

Thanks for confirmation! Then I think we should move forward with these changes and just keep an eye on the issues list in case users report a disruption. We can add the ABI check before the release if we need to.

[rjmccall]

IIRC, the original point of this function was to expose a way to get the canonical type extremely cheaply for operations that are just going to ignore local qualifiers anyway. The problem is not that that operation exists — it's good to use a cheaper operation when you can — it's that it's misnamed in a way that's encouraging misuse. We should probably break it into getUnqualifiedCanonicalType, which does the change you're proposing here, and getCanonicalTypeWithoutLocalQualifiers(), which hopefully has a sufficiently discouraging name that people won't accidentally use it.

[rosefromthedead]

That works for me, too. I was wary that that approach might not be worth the churn, but it could make for a more sane interface.

[rosefromthedead]

Actually, I think the performance cost is negligible, so it's not really worth splitting into two. What do you think?

[rjmccall]

getCanonicalTypeWithoutLocalQualifiers is basically just loading the canonical type field from the Type object, so it's an extremely cheap operation for what it is, whereas the other is a non-trivial operation that at least needs a call. Are you arguing that it just doesn't matter in practice?