[Clang] remove qualifiers in getCanonicalTypeUnqualified#170271
[Clang] remove qualifiers in getCanonicalTypeUnqualified#170271AaronBallman merged 2 commits intollvm:mainfrom
Conversation
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 llvm#135273
|
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 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. |
cor3ntin
left a comment
There was a problem hiding this comment.
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!
|
@llvm/pr-subscribers-clang Author: Rose Hudson (rosefromthedead) ChangesIt 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:
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
}
}
|
AaronBallman
left a comment
There was a problem hiding this comment.
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.
|
@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. |
|
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 |
|
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. |
|
Actually, I think the performance cost is negligible, so it's not really worth splitting into two. What do you think? |
|
|
|
I think it isn't a significant slowdown for I also think it's not a significant slowdown for clang because I'm assuming that this function isn't hot, but I haven't checked. |
|
@rjmccall please could you clarify: do you consider these concerns blocking for this patch? |
|
Well, I don't have any evidence that this is specifically hot, so I won't insist. Go ahead. |
|
@mizvekov @AaronBallman with the above in mind, does the patch still look good to you? If so, please merge it. I don't have commit rights. |
|
@rosefromthedead Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/20140 Here is the relevant piece of the build log for the reference |
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