[Clang][Sema] set declaration invalid earlier to prevent crash in calculating record layout

[jcsxky]

Try to fix #75221
This crash caused by calculating record layout which contains a field declaration with dependent type. Make it invalid before it is a complete definition to prevent this crash. Define a new scope type to record this type alias and set the record declaration invalid when it is defined in a type alias template.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Try to fix #75221
This crash caused by calculating record layout which contains a field declaration with dependent type. Make it invalid when report diagnose to prevent this crash. Set the record declaration incomplete bypass the assertion and restore the status when finish setting it invalid.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaType.cpp (+3)
• (added) clang/test/SemaCXX/PR75221.cpp (+7)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 37b843915a0dee..20578c9b60e33c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -465,6 +465,8 @@ Bug Fixes to C++ Support
   following the first `::` were ignored).
 - Fix an out-of-bounds crash when checking the validity of template partial specializations. (part of #GH86757).
 - Fix an issue caused by not handling invalid cases when substituting into the parameter mapping of a constraint. Fixes (#GH86757).
+- Fix a crash caused by defined struct in a type alias template when the structure
+  has fields with dependent type. Fixes (#GH75221).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index fd94caa4e1d449..973ad20c943bde 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -3899,6 +3899,9 @@ static QualType GetDeclSpecTypeForDeclarator(TypeProcessingState &state,
       SemaRef.Diag(OwnedTagDecl->getLocation(), DiagID)
           << SemaRef.Context.getTypeDeclType(OwnedTagDecl);
       D.setInvalidType(true);
+      OwnedTagDecl->setCompleteDefinition(false);
+      OwnedTagDecl->setInvalidDecl();
+      OwnedTagDecl->setCompleteDefinition();
     }
   }
 
diff --git a/clang/test/SemaCXX/PR75221.cpp b/clang/test/SemaCXX/PR75221.cpp
new file mode 100644
index 00000000000000..08b7a06676a8a5
--- /dev/null
+++ b/clang/test/SemaCXX/PR75221.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -verify -std=c++11 -fsyntax-only %s
+// expected-no-diagnostics
+
+template <class T> using foo = struct foo {
+  T size = 0;
+};
+foo a;

[erichkeane]

Are we sure the OwnedTagDecl is ALWAYS a complete definition here? Should we cache the existing value before 3902 and restore that rather than always setting it complete?

[shafik]

I am little uncomfortable with this change here, I see that you are calling setCompleteDefinition(false) in order to get around the assert that the decl is not complete in Decl::setInvalidDecl(...). If this precondition is not true we need to understand why it break down or if this implies we should be handling this differently.

[shafik]

GetDeclSpecTypeForDeclarator is called from two places GetTypeForDeclaratorCast(...) and where that is called D.isInvalidType() is checked. It is also called from GetTypeForDeclarator(...) and the checking of after that call is a bit more varied. I am wondering if we are being sloppy in one of those calls.

[jcsxky]

@erichkeane Condition holds in outer if(line 3841) indicates OwnedTagDecl is a complete definition.

[jcsxky]

@shafik If I understand correctly, you want to set OwnedTagDecl invalid before it is a complete definition. I will look into the code.

[jcsxky]

#0  clang::TagDecl::setCompleteDefinition (this=0x5555556cabd8, V=true) at /home/huqizhi/llvm-project/clang/include/clang/AST/Decl.h:3656
#1  0x00007fffe775b540 in clang::TagDecl::completeDefinition (this=0x5555556cabd8) at /home/huqizhi/llvm-project/clang/lib/AST/Decl.cpp:4720
#2  0x00007fffe775d110 in clang::RecordDecl::completeDefinition (this=0x5555556cabd8) at /home/huqizhi/llvm-project/clang/lib/AST/Decl.cpp:5049
#3  0x00007fffe779be00 in clang::CXXRecordDecl::completeDefinition (this=0x5555556cabd8, FinalOverriders=0x0) at /home/huqizhi/llvm-project/clang/lib/AST/DeclCXX.cpp:2059
#4  0x00007fffe779bdd9 in clang::CXXRecordDecl::completeDefinition (this=0x5555556cabd8) at /home/huqizhi/llvm-project/clang/lib/AST/DeclCXX.cpp:2055
#5  0x00007fffe44cbab2 in clang::Sema::ActOnFields (this=0x5555556b7060, S=0x5555556d6700, RecLoc=..., EnclosingDecl=0x5555556cabd8, Fields=..., LBrac=..., RBrac=..., Attrs=...)
    at /home/huqizhi/llvm-project/clang/lib/Sema/SemaDecl.cpp:19654
#6  0x00007fffe4636eed in clang::Sema::ActOnFinishCXXMemberSpecification (this=0x5555556b7060, S=0x5555556d6700, RLoc=..., TagDecl=0x5555556cabd8, LBrac=..., RBrac=..., AttrList=...)
    at /home/huqizhi/llvm-project/clang/lib/Sema/SemaDeclCXX.cpp:10540
#7  0x00007fffe5956cf2 in clang::Parser::ParseCXXMemberSpecification (this=0x5555556c4580, RecordLoc=..., AttrFixitLoc=..., Attrs=..., TagType=25, TagDecl=0x5555556cabd8)
    at /home/huqizhi/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:3782
#8  0x00007fffe5954f8e in clang::Parser::ParseClassSpecifier (this=0x5555556c4580, TagTokKind=clang::tok::kw_struct, StartLoc=..., DS=..., TemplateInfo=..., AS=clang::AS_none, EnteringContext=false, 
    DSC=clang::Parser::DeclSpecContext::DSC_alias_declaration, Attributes=...) at /home/huqizhi/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:2252
#9  0x00007fffe591b23e in clang::Parser::ParseDeclarationSpecifiers (this=0x5555556c4580, DS=..., TemplateInfo=..., AS=clang::AS_none, DSContext=clang::Parser::DeclSpecContext::DSC_alias_declaration, 
    LateAttrs=0x0, AllowImplicitTypename=clang::ImplicitTypenameContext::Yes) at /home/huqizhi/llvm-project/clang/lib/Parse/ParseDecl.cpp:4553
#10 0x00007fffe59167a3 in clang::Parser::ParseSpecifierQualifierList (this=0x5555556c4580, DS=..., AllowImplicitTypename=clang::ImplicitTypenameContext::Yes, AS=clang::AS_none, 
    DSC=clang::Parser::DeclSpecContext::DSC_alias_declaration) at /home/huqizhi/llvm-project/clang/lib/Parse/ParseDecl.cpp:2829
#11 0x00007fffe592bea4 in clang::Parser::ParseSpecifierQualifierList (this=0x5555556c4580, DS=..., AS=clang::AS_none, DSC=clang::Parser::DeclSpecContext::DSC_alias_declaration)
    at /home/huqizhi/llvm-project/clang/include/clang/Parse/Parser.h:2480
#12 0x00007fffe5906906 in clang::Parser::ParseTypeName (this=0x5555556c4580, Range=0x0, Context=clang::DeclaratorContext::AliasTemplate, AS=clang::AS_none, OwnedType=0x7fffffff8a88, 
    Attrs=0x7fffffff90b0) at /home/huqizhi/llvm-project/clang/lib/Parse/ParseDecl.cpp:59

From the trace we can see that if we want to set OwnedTagDecl invalid before it's a complete definition, Context (DeclaratorContext)should be passed through these funtions (I haven't found how to get this information without passing Context as of now). It's not a better approach than current version from my side. @shafik WDYT?

[erichkeane]

It DEFINITELY seems strange to me that we're marking this 'complete' before we know if it is valid. So from that perspective, this patch is in the 'wrong' place.

[jcsxky]

gently ping. @erichkeane @shafik Any opinions on this pr?