Skip to content

[clang] Fix cast for injected types in case name lookup for dependent bases #119024

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.

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

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

vbe-sc
Copy link
Contributor

@vbe-sc vbe-sc commented Dec 6, 2024

An assertion failure occurs in Clang when attempting to compile such an example:

template <typename, typename, bool> struct MozPromise {
  class Private;

private:
  int mMagic4 = 42;
};

template <typename ResolveValueT, typename RejectValueT, bool IsExclusive>
struct MozPromise<ResolveValueT, RejectValueT, IsExclusive>::Private : MozPromise {
  void SetTaskPriority() { mMagic4 ; }
};

Output:

clang: llvm-project/llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&) [with To = clang::RecordType; From = clang::QualType]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

The reason is in the incorrect way of casting types when searching for names in base classes

return Specifier->getType()->castAs<RecordType>()->getDecl()->getCanonicalDecl() == BaseRecord;

It loses injected types for template class names.

This patch provides fix for such cases

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 6, 2024

@llvm/pr-subscribers-clang

Author: Vladislav Belov (vbe-sc)

Changes

An assertion failure occurs in Clang when attempting to compile such an example:

template &lt;typename, typename, bool&gt; struct MozPromise {
  class Private;

private:
  int mMagic4 = 42;
};

template &lt;typename ResolveValueT, typename RejectValueT, bool IsExclusive&gt;
struct MozPromise&lt;ResolveValueT, RejectValueT, IsExclusive&gt;::Private : MozPromise {
  void SetTaskPriority() { mMagic4 ; }
};

Output:

clang: llvm-project/llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&amp;) [with To = clang::RecordType; From = clang::QualType]: Assertion `isa&lt;To&gt;(Val) &amp;&amp; "cast&lt;Ty&gt;() argument of incompatible type!"' failed.

The reason is in the incorrect way of casting types when searching for names in base classes

return Specifier-&gt;getType()-&gt;castAs&lt;RecordType&gt;()-&gt;getDecl()-&gt;getCanonicalDecl() == BaseRecord;

It loses injected types for template class names.

This patch provides fix for such cases


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

1 Files Affected:

  • (modified) clang/lib/AST/CXXInheritance.cpp (+6-6)
diff --git a/clang/lib/AST/CXXInheritance.cpp b/clang/lib/AST/CXXInheritance.cpp
index 10b8d524ff8978..74848696963e99 100644
--- a/clang/lib/AST/CXXInheritance.cpp
+++ b/clang/lib/AST/CXXInheritance.cpp
@@ -368,8 +368,8 @@ bool CXXRecordDecl::FindBaseClass(const CXXBaseSpecifier *Specifier,
                                   const CXXRecordDecl *BaseRecord) {
   assert(BaseRecord->getCanonicalDecl() == BaseRecord &&
          "User data for FindBaseClass is not canonical!");
-  return Specifier->getType()->castAs<RecordType>()->getDecl()
-            ->getCanonicalDecl() == BaseRecord;
+  return (cast<CXXRecordDecl>(Specifier->getType()->getAsRecordDecl())
+              ->getCanonicalDecl()) == BaseRecord;
 }
 
 bool CXXRecordDecl::FindVirtualBaseClass(const CXXBaseSpecifier *Specifier,
@@ -378,8 +378,8 @@ bool CXXRecordDecl::FindVirtualBaseClass(const CXXBaseSpecifier *Specifier,
   assert(BaseRecord->getCanonicalDecl() == BaseRecord &&
          "User data for FindBaseClass is not canonical!");
   return Specifier->isVirtual() &&
-         Specifier->getType()->castAs<RecordType>()->getDecl()
-            ->getCanonicalDecl() == BaseRecord;
+         (cast<CXXRecordDecl>(Specifier->getType()->getAsRecordDecl())
+              ->getCanonicalDecl()) == BaseRecord;
 }
 
 static bool isOrdinaryMember(const NamedDecl *ND) {
@@ -692,7 +692,7 @@ AddIndirectPrimaryBases(const CXXRecordDecl *RD, ASTContext &Context,
            "Cannot get indirect primary bases for class with dependent bases.");
 
     const CXXRecordDecl *BaseDecl =
-      cast<CXXRecordDecl>(I.getType()->castAs<RecordType>()->getDecl());
+        cast<CXXRecordDecl>(I.getType()->getAsRecordDecl());
 
     // Only bases with virtual bases participate in computing the
     // indirect primary virtual base classes.
@@ -714,7 +714,7 @@ CXXRecordDecl::getIndirectPrimaryBases(CXXIndirectPrimaryBaseSet& Bases) const {
            "Cannot get indirect primary bases for class with dependent bases.");
 
     const CXXRecordDecl *BaseDecl =
-      cast<CXXRecordDecl>(I.getType()->castAs<RecordType>()->getDecl());
+        cast<CXXRecordDecl>(I.getType()->getAsRecordDecl());
 
     // Only bases with virtual bases participate in computing the
     // indirect primary virtual base classes.

@vbe-sc vbe-sc force-pushed the vb-sc/clang-dependent-lookup-cast-fix branch from 9c501fc to e3fd6b1 Compare December 7, 2024 11:31
@vbe-sc vbe-sc requested a review from Endilll as a code owner December 7, 2024 11:31
@vbe-sc
Copy link
Contributor Author

vbe-sc commented Dec 7, 2024

@erichkeane, @cor3ntin, could you, please, take a look at this fix?

@cor3ntin cor3ntin changed the title [clang][NFC] Fix cast for injected types in case name lookup for dependent bases [clang] Fix cast for injected types in case name lookup for dependent bases Dec 7, 2024
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

Can you confirm this is a fix for #118003 ?
In which case we do not need changelog entry

Comment on lines 371 to 372
return (cast<CXXRecordDecl>(Specifier->getType()->getAsRecordDecl())
->getCanonicalDecl()) == BaseRecord;
Copy link
Contributor

Choose a reason for hiding this comment

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

You do not need the outer parentheses here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@vbe-sc vbe-sc force-pushed the vb-sc/clang-dependent-lookup-cast-fix branch from e3fd6b1 to ce758d5 Compare December 8, 2024 10:16
@vbe-sc
Copy link
Contributor Author

vbe-sc commented Dec 8, 2024

Can you confirm this is a fix for #118003 ? In which case we do not need changelog entry

Yes, you are right

@vbe-sc vbe-sc requested a review from cor3ntin December 8, 2024 14:10
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM

@cor3ntin cor3ntin merged commit 01710aa into llvm:main Dec 9, 2024
8 checks passed
@vbe-sc
Copy link
Contributor Author

vbe-sc commented Dec 9, 2024

@glandium, this is the fix for your example. Please, feel free to contact if you have any more troubles with this patch

@@ -368,8 +368,8 @@ bool CXXRecordDecl::FindBaseClass(const CXXBaseSpecifier *Specifier,
const CXXRecordDecl *BaseRecord) {
assert(BaseRecord->getCanonicalDecl() == BaseRecord &&
"User data for FindBaseClass is not canonical!");
return Specifier->getType()->castAs<RecordType>()->getDecl()
->getCanonicalDecl() == BaseRecord;
return cast<CXXRecordDecl>(Specifier->getType()->getAsRecordDecl())
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this instead of:

Specifier->getType()->getAsCXXRecordDecl() ??

Same question throughout this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants