[ms] CXXRecordDecl::getMSInheritanceModel() dereferences null MSInheritanceAttr #36747
Comments
|
The way that I would've expected this to work is for assignInheritanceModel to apply the attribute to the most recent declaration, and every re-declaration of the class would naturally inherit the attribute. I think most attributes are inherited by redeclarations, and this should be no different. Was this a recent regression? CRTP with member function pointers is nothing new. I vaguely recall making sure this pattern worked in the past. |
|
We hit this in the v6.0.0 binary release of clange/LLVM for Win32 - I haven't tried to reproduce with older versions. I am a clang newbie - and will have to look more at how attributes are inherited... Thanks, Andrew R |
|
I've done some archaeology using the binary releases - it appears this was introduced between 3.7.1 and 3.8.0: C:\Temp\clang\clang>\llvm-3.7\bin\clang-cl.exe -cc1 -emit-llvm-only -x c++ -v test.cpp C:\Temp\clang\clang>\llvm-3.7.1\bin\clang-cl.exe -cc1 -emit-llvm-only -x c++ -v test.cpp C:\Temp\clang\clang>\llvm-3.8\bin\clang-cl.exe -cc1 -emit-llvm-only -x c++ -v test.cpp Thanks, Andrew R |
|
Adding my 2c here: I'd have thought that calculateInheritanceModel() handles this case by checking isParsingBaseSpecifiers(). |
|
I think what's going on here is that the injected class name CXXRecordDecl redeclaration doesn't inherit most attributes. I'm not yet sure where that gets created, but we might want to skip injected class names when we assign inheritance and look it up if it doesn't copy attributes from previous declarations. |
|
Hi Reid, You're right - the key issue here is that in this case the injected-class-name is added into the decl list (clang::Redeclarableclang::TagDecl) for the DerivedFunctor after we've called assignInheritanceModel() during parsing of PtrToMemberFunction. I took a look at how the following (roughly equivalent) non-template code is compiled: class Foo; class Functor class Bar class Foo Foo AFunctor; In it, the injected-class-name is still created after we've called assignInheritanceModel() during parsing of PtrToMemberFunction, but it isn't added into the decl list (clang::Redeclarableclang::TagDecl) for the class Foo. In particular, this is because when not instantiating class templates, the PrevDecl passed to create the CXXRecordDecl for an injected-class-name is nullptr (c.f. below from Sema::ActOnStartCXXMemberDeclarations()): // C++ [class]p2: This compares with the following code in TemplateDeclInstantiator::VisitCXXRecordDecl(), which creates both the class declarations and the injected-class-name: CXXRecordDecl *PrevDecl = nullptr; CXXRecordDecl *Record I have briefly experimented with changing the first couple lines to: CXXRecordDecl *PrevDecl; so that the injected-class-name for a class template instantiation isn't linked into the decl list for the instantiated class type and this then prevents the issue I reported happening and gives the injected-class-name for a class template instantiation similar semantics to the injected-class-name for a normal class - it also seems like a cleaner fix. I haven't had chance to run a broader test suite yet, but do you think this makes sense as a route to fixing this problem? Thanks, Andrew R |
|
To respond to David's point about isParsingBaseSpecifiers(), it should be noted that isParsingBaseSpecifiers() is only true during a call to Parser::ParseBaseClause(), with: setting isParsingBaseSpecifiers() to true and: setting isParsingBaseSpecifiers() back to false. Parser::ParseBaseClause() is called as the template declaration is parsed: and well before assignInheritanceModel() is called, during the parsing of the declaration of AFunctor, which causes the implicit instantiation of DerivedFunctor: RD = class DerivedFunctor Therefore, since the template declaration (including its base sepcifiers) is parsed well before the declaration which triggers the template instantiation (and template instantiation does not trigger further parsing of base specifiers), isParsingBaseSpecifiers() isn't of any use in this situation. Thanks, Andrew R |
|
Hi Reid, I'm still finding my feet, so apologies here... I've worked out how to run the SemaTemplate tests and they make clear that the idea of changing the injected-class-name for the template instantiation to have a nullptr PrevDecl isn't a good idea (causes 43/225 tests to fail!). The good news is that the original fix I suggested doesn't cause any test regressions - though I agree the naming could be improved... Since the injected-class-name is a special case where PrevDecl is Owner, rather than found via using the pattern from getPreviousDeclForInstantiation(D), how about restricting the call to just injected-class-name decls and renaming to e.g: if (D->isInjectedClassName()) [that's now a lot more accurate, but a bit of a mouth full!] This change then still fixes my original problem and causes no regressions in the 225 SemaTemplate tests - what do you think? If you are happy, I will rebase and update with a patch containing this new naming. Thanks, Andrew R |
|
I think there's two ways to go on this issue:
I think propagating the attributes to the injected class name is also a workaround, since it doesn't move towards the long term solution. If we're going to have a workaround, I think we should introduce a new CXXRecordDecl::getMostNonInjectedDecl method that gets the most recent non-injected class name redeclaration, and then we'll use that instead when we apply implicit attributes and look for inheritance attributes. |
|
Attempt to use non-injected name decl |
|
Hi Reid, As per my last comment, the patch attached will fix the original problem I reported, I hope using the strategy you envisioned. However the following slightly expanded example: |
|
Hi Reid, As per the previous comment, I created a patch which attempts to make the fix you suggested of just using the non-injected name decl whenever the MS inheritance attribute is accessed or set. This patch fixes the case I originally reported. However, the following slightly expanded example code: template template class DerivedFunctor; template DerivedFunctor BFunctor; int main() Then fails to compile: c:\temp\clang\clang\test.cpp:22:33: error: incomplete type 'void (DerivedFunctor::*)()' is not assignable |
|
Hi Reid, I found I'd missed one case in Type::isIncompleteType(). Adding it fixed the code I quoted and all clang tests passed with this patch. I've put the new patch on https://reviews.llvm.org/D46664. Thanks, Andrew R |
|
Fixed by rC333680: |
Extended Description
Compile the following:
template
class Functor
{
void (ObjT::*PtrToMemberFunction)();
};
template class DerivedFunctor;
template
class DerivedFunctor
: public Functor<DerivedFunctor >
{
};
DerivedFunctor AFunctor;
with:
clang-cpp.exe -cc1 -emit-llvm-only -x c++ test.cpp
This generates the following ASSERT (debug build) / AV (release build):
C:\Temp\clang\clang>c:\workspaces\llvm-build\Debug\bin\clang-cpp.exe -cc1 -emit-llvm-only -x c++ test.cpp
Assertion failed: IA && "Expected MSInheritanceAttr on the CXXRecordDecl!", file c:\workspaces\llvm\tools\clang\lib\ast\microsoftcxxabi.cpp, line 171
Wrote crash dump file "C:\Users\14166\AppData\Local\Temp\clang-cpp.exe-346498.dmp"
...
The failing callstack which tries to use a null MSInheritanceAttr looks like this:
The AST looks like this (with -ast-dump):
TranslationUnitDecl 0xc7e6ad0 <>
|-TypedefDecl 0xc7e6fe8 <> implicit __NSConstantString '__NSConstantString_tag'
|
-RecordType 0xc7e6e40 '__NSConstantString_tag' |-CXXRecord 0xc7e6de0 '__NSConstantString_tag'|-CXXRecordDecl 0xc7e7018 <> implicit class type_info
|
-TypeVisibilityAttr 0xc7e7098 <<invalid sloc>> Implicit Default |-TypedefDecl 0xc7e70e0 <<invalid sloc>> <invalid sloc> implicit size_t 'unsigned int' |-BuiltinType 0xc7e6ba0 'unsigned int'|-TypedefDecl 0xc7e7140 <> implicit __builtin_va_list 'char '
|
-PointerType 0xc7e7110 'char *' |-BuiltinType 0xc7e6b20 'char'|-ClassTemplateDecl 0xc7e7250 <test.cpp:1:1, line:5:1> line:2:7 Functor
| |-TemplateTypeParmDecl 0xc7e7170 <line:1:10, col:19> col:19 typename depth 0 index 0 ObjT
| |-CXXRecordDecl 0xc7e71f0 <line:2:1, line:5:1> line:2:7 class Functor definition
| | |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial
| | | |-DefaultConstructor exists trivial needs_implicit
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | |
-Destructor simple irrelevant trivial needs_implicit | | |-CXXRecordDecl 0xc7e73f8 <col:1, col:7> col:7 implicit class Functor | |-FieldDecl 0xc7e7558 <line:4:4, col:38> col:17 PtrToMemberFunction 'void (ObjT::)() attribute((thiscall))'|
-ClassTemplateSpecializationDecl 0xe986ad0 <line:1:1, line:5:1> line:2:7 class Functor definition | |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial literal | | |-DefaultConstructor exists trivial | | |-CopyConstructor simple trivial has_const_param implicit_has_const_param | | |-MoveConstructor exists simple trivial | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param | | |-MoveAssignment exists simple trivial needs_implicit | |-Destructor simple irrelevant trivial| |-TemplateArgument type 'DerivedFunctor'
| |-CXXRecordDecl 0xe986c40 prev 0xe986ad0 <col:1, col:7> col:7 implicit class Functor
| |-FieldDecl 0xe986d70 <line:4:4, col:38> col:17 PtrToMemberFunction 'void (DerivedFunctor::*)() attribute((thiscall))'
| |-CXXConstructorDecl 0xe986f10 line:2:7 col:7 implicit used Functor 'void () attribute((thiscall)) noexcept' inline default trivial
| |
-CompoundStmt 0xe9874e0 <col:7> | |-CXXDestructorDecl 0xe986fb0 <col:7> col:7 implicit ~Functor 'void () __attribute__((thiscall))' inline default trivial noexcept-unevaluated 0xe986fb0 | |-CXXConstructorDecl 0xe987078 <col:7> col:7 implicit constexpr Functor 'void (const Functor<DerivedFunctor<void> > &) __attribute__((thiscall))' inline default trivial noexcept-unevaluated 0xe987078 | |-ParmVarDecl 0xe987138 col:7 col:7 'const Functor<DerivedFunctor > &'|
-CXXConstructorDecl 0xe987198 <col:7> col:7 implicit constexpr Functor 'void (Functor<DerivedFunctor<void> > &&) __attribute__((thiscall))' inline default trivial noexcept-unevaluated 0xe987198 |-ParmVarDecl 0xe987258 col:7 col:7 'Functor<DerivedFunctor > &&'|-ClassTemplateDecl 0xc7e7650 <line:7:1, col:35> col:35 DerivedFunctor
| |-TemplateTypeParmDecl 0xc7e7598 <col:10, col:19> col:19 typename depth 0 index 0 SomeType
| |-CXXRecordDecl 0xc7e75f0 <col:29, col:35> col:35 class DerivedFunctor
|
-ClassTemplateSpecializationDecl 0xe986840 <line:9:1, line:13:1> line:10:7 class DerivedFunctor definition | |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial literal | | |-DefaultConstructor exists trivial | | |-CopyConstructor simple trivial has_const_param implicit_has_const_param | | |-MoveConstructor exists simple trivial | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param | | |-MoveAssignment exists simple trivial needs_implicit | |-Destructor simple irrelevant trivial needs_implicit| |-public 'Functor<DerivedFunctor >':'Functor<DerivedFunctor >'
| |-TemplateArgument type 'void'
| |-MSInheritanceAttr 0xe986d40 <line:9:1, line:13:1> Implicit __single_inheritance BestCase
| |-CXXRecordDecl 0xe986de0 prev 0xe986840 <line:10:1, col:7> col:7 implicit class DerivedFunctor
| |-CXXConstructorDecl 0xe986e60 col:7 col:7 implicit used DerivedFunctor 'void () attribute((thiscall)) noexcept' inline default trivial
| | |-CXXCtorInitializer 'Functor<DerivedFunctor >':'Functor<DerivedFunctor >'
| | |
-CXXConstructExpr 0xe9874f0 <col:7> 'Functor<DerivedFunctor<void> >':'Functor<DerivedFunctor<void> >' 'void () __attribute__((thiscall)) noexcept' | |-CompoundStmt 0xe987590 col:7| |-CXXConstructorDecl 0xe987298 col:7 col:7 implicit constexpr DerivedFunctor 'void (const DerivedFunctor &) attribute((thiscall))' inline default trivial noexcept-unevaluated 0xe987298
| |
-ParmVarDecl 0xe987358 <col:7> col:7 'const DerivedFunctor<void> &' |-CXXConstructorDecl 0xe9873b8 col:7 col:7 implicit constexpr DerivedFunctor 'void (DerivedFunctor &&) attribute((thiscall))' inline default trivial noexcept-unevaluated 0xe9873b8|
-ParmVarDecl 0xe987478 <col:7> col:7 'DerivedFunctor<void> &&' |-ClassTemplateDecl 0xc7e7860 prev 0xc7e7650 <line:9:1, line:13:1> line:10:7 DerivedFunctor | |-TemplateTypeParmDecl 0xc7e77a8 <line:9:10, col:19> col:19 referenced typename depth 0 index 0 SomeType | |-CXXRecordDecl 0xc7e7800 prev 0xc7e75f0 <line:10:1, line:13:1> line:10:7 class DerivedFunctor definition | | |-DefinitionData pass_in_registers empty standard_layout trivially_copyable trivial literal has_constexpr_non_copy_move_ctor can_const_default_init | | | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr | | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param | | | |-MoveConstructor exists simple trivial needs_implicit | | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param | | | |-MoveAssignment exists simple trivial needs_implicit | | |-Destructor simple irrelevant trivial needs_implicit| | |-public 'Functor<DerivedFunctor >'
| |
-CXXRecordDecl 0xe9867c0 <col:1, col:7> col:7 implicit class DerivedFunctor |-ClassTemplateSpecialization 0xe986840 'DerivedFunctor'-VarDecl 0xe986988 <line:15:1, col:22> col:22 AFunctor 'DerivedFunctor<void>':'DerivedFunctor<void>' callinit-CXXConstructExpr 0xe9875a0 col:22 'DerivedFunctor':'DerivedFunctor' 'void () attribute((thiscall)) noexcept'The problem occurs because assignInheritanceModel() is called on the "class DerivedFunctor" CXXRecordDecl, when it is the RD returned by MPTy->getMostRecentCXXRecordDecl():
but then the "implicit class DerivedFunctor" CXXRecordDecl is added as the 'latest' decl (returned by MPTy->getMostRecentCXXRecordDecl()), as the template is instantiated:
Since the "implicit class DerivedFunctor" CXXRecordDecl doesn't have a MSInheritanceAttr (but the "class DerivedFunctor" CXXRecordDecl does), when the "implicit class DerivedFunctor" CXXRecordDecl is later selected by MPTy->getMostRecentCXXRecordDecl() no MSInheritanceAttr can be found.
I debugged this and TemplateDeclInstantiator::VisitCXXRecordDecl() looks like an ideal place to move information forward from the "class DerivedFunctor" CXXRecordDecl to the "implicit class DerivedFunctor" CXXRecordDecl. I have created a fix which calls back into the semantic analysis code and check if the previous decl has a MSInheritanceAttr, and if so to just add a MSInheritanceAttr to the new decl being created.
This fix resolves this crash, which was hit over multiple different source files in some code I'mm porting to build with clang / MS ABI.
The text was updated successfully, but these errors were encountered: