Thank you for the analysis! It would be nice if you could implement a short term workaround to avoid crashes. Maybe Richard has ideas on what a proper solution should look like.
On Wed, Aug 2, 2017 at 8:05 PM, Serge Pavlov <sepavl...@gmail.com> wrote: > At first thank you for the nice test case. > > This crash is not caused by r305903. The root cause is that instantiation > of f<int> is triggered when parse of f<T> is not finished yet. This is the > case just addressed by that change. > The instantiation is requested by the code (https://github.com/llvm-mirro > r/clang/blob/master/lib/Sema/SemaExpr.cpp#L13622): > > else if (Func->isConstexpr()) > // Do not defer instantiations of constexpr functions, to avoid the > // expression evaluator needing to call back into Sema if it sees a > // call to such a function. > InstantiateFunctionDefinition(PointOfInstantiation, Func); > > > It looks like a violation of C++ Standard, as function is instantiated > when there is no use of it. However if this check is removed and > instantiation is postponed as for non-constexpr functions, lots of tests > fail. > > Without r305903 the call to `getDefinition` for templated declaration of > f<T> returned null, declaration f<int> was moved to the list of pending > instantiations and crash is not observed. It does not mean however that > original source code would be compiled correctly. > > It this is urgent problem, I can prepare a patch that will mimic behavior > before r305903 for instantiation of constexpr functions. If there is some > time I would investigate this problem in depth, as it manifests itself in > number of cases, for instance in https://bugs.llvm.org/show_ > bug.cgi?id=33561. > > Thanks, > --Serge > > 2017-08-01 23:28 GMT+07:00 Serge Pavlov <sepavl...@gmail.com>: > >> Yes, sure, I will investigate it. >> >> Thanks, >> --Serge >> >> 2017-08-01 21:32 GMT+07:00 Alexander Kornienko <ale...@google.com>: >> >>> This change causes an assertion failure on valid code. Could you take a >>> look at fixing this? >>> >>> A reduced test case: >>> >>> $ cat /tmp/SemaTemplateInstantiateDecl-crash2.cpp >>> template <class T> >>> constexpr void f(T) { >>> f(0); >>> } >>> $ clang -fsyntax-only -std=c++11 /tmp/SemaTemplateInstantiateDe >>> cl-crash2.cpp >>> assert.h assertion failed at llvm/tools/clang/lib/Sema/Sema >>> TemplateInstantiateDecl.cpp:3840 in void clang::Sema::InstantiateFuncti >>> onDefinition(clang::SourceLocation, clang::FunctionDecl *, bool, bool, >>> bool): (Pattern | >>> | PatternDecl->isDefaulted()) && "unexpected kind of function template >>> definition" >>> *** Check failure stack trace: *** >>> @ 0x6094c4a __assert_fail >>> @ 0x2705bfe clang::Sema::InstantiateFunctionDefinition() >>> @ 0x2c1fb13 clang::Sema::MarkFunctionReferenced() >>> @ 0x2bec07b clang::Sema::MarkAnyDeclReferenced() >>> @ 0x2c2327f MarkExprReferenced() >>> @ 0x2be8f0a clang::Sema::MarkDeclRefReferenced() >>> @ 0x2980ac0 clang::Sema::FixOverloadedFunctionReference() >>> @ 0x2982e66 FinishOverloadedCallExpr() >>> @ 0x2982b39 clang::Sema::BuildOverloadedCallExpr() >>> @ 0x2be461b clang::Sema::ActOnCallExpr() >>> @ 0x2571e90 clang::Parser::ParsePostfixExpressionSuffix() >>> @ 0x25784cb clang::Parser::ParseCastExpression() >>> @ 0x2570865 clang::Parser::ParseCastExpression() >>> @ 0x256f1e3 clang::Parser::ParseAssignmentExpression() >>> @ 0x256f0bf clang::Parser::ParseExpression() >>> @ 0x2517eeb clang::Parser::ParseExprStatement() >>> @ 0x2517074 clang::Parser::ParseStatement >>> OrDeclarationAfterAttributes() >>> @ 0x2516b50 clang::Parser::ParseStatementOrDeclaration() >>> @ 0x251deb4 clang::Parser::ParseCompoundStatementBody() >>> @ 0x251ea53 clang::Parser::ParseFunctionStatementBody() >>> @ 0x24f5b23 clang::Parser::ParseFunctionDefinition() >>> @ 0x25082a5 clang::Parser::ParseSingleDec >>> larationAfterTemplate() >>> @ 0x2507652 clang::Parser::ParseTemplateD >>> eclarationOrSpecialization() >>> @ 0x2506fa5 clang::Parser::ParseDeclarati >>> onStartingWithTemplate() >>> @ 0x25b6853 clang::Parser::ParseDeclaration() >>> @ 0x24f36d5 clang::Parser::ParseExternalDeclaration() >>> @ 0x24f2739 clang::Parser::ParseTopLevelDecl() >>> @ 0x24f220e clang::Parser::ParseFirstTopLevelDecl() >>> @ 0x24ed582 clang::ParseAST() >>> >>> On Wed, Jun 21, 2017 at 2:46 PM, Serge Pavlov via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> >>>> Author: sepavloff >>>> Date: Wed Jun 21 07:46:57 2017 >>>> New Revision: 305903 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=305903&view=rev >>>> Log: >>>> Function with unparsed body is a definition >>>> >>>> While a function body is being parsed, the function declaration is not >>>> considered >>>> as a definition because it does not have a body yet. In some cases it >>>> leads to >>>> incorrect interpretation, the case is presented in >>>> https://bugs.llvm.org/show_bug.cgi?id=14785: >>>> ``` >>>> template<typename T> struct Somewhat { >>>> void internal() const {} >>>> friend void operator+(int const &, Somewhat<T> const &) {} >>>> }; >>>> void operator+(int const &, Somewhat<char> const &x) { x.internal(); } >>>> ``` >>>> When statement `x.internal()` in the body of global `operator+` is >>>> parsed, the type >>>> of `x` must be completed, so the instantiation of `Somewhat<char>` is >>>> started. It >>>> instantiates the declaration of `operator+` defined inline, and makes a >>>> check for >>>> redefinition. The check does not detect another definition because the >>>> declaration >>>> of `operator+` is still not defining as does not have a body yet. >>>> >>>> To solves this problem the function `isThisDeclarationADefinition` >>>> considers >>>> a function declaration as a definition if it has flag `WillHaveBody` >>>> set. >>>> >>>> This change fixes PR14785. >>>> >>>> Differential Revision: https://reviews.llvm.org/D30375 >>>> >>>> This is a recommit of 305379, reverted in 305381, with small changes. >>>> >>>> Modified: >>>> cfe/trunk/include/clang/AST/Decl.h >>>> cfe/trunk/lib/Sema/SemaCUDA.cpp >>>> cfe/trunk/lib/Sema/SemaDecl.cpp >>>> cfe/trunk/lib/Sema/SemaDeclCXX.cpp >>>> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp >>>> cfe/trunk/test/SemaCXX/friend2.cpp >>>> >>>> Modified: cfe/trunk/include/clang/AST/Decl.h >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >>>> AST/Decl.h?rev=305903&r1=305902&r2=305903&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/include/clang/AST/Decl.h (original) >>>> +++ cfe/trunk/include/clang/AST/Decl.h Wed Jun 21 07:46:57 2017 >>>> @@ -1874,7 +1874,7 @@ public: >>>> /// >>>> bool isThisDeclarationADefinition() const { >>>> return IsDeleted || IsDefaulted || Body || IsLateTemplateParsed || >>>> - hasDefiningAttr(); >>>> + WillHaveBody || hasDefiningAttr(); >>>> } >>>> >>>> /// doesThisDeclarationHaveABody - Returns whether this specific >>>> >>>> Modified: cfe/trunk/lib/Sema/SemaCUDA.cpp >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC >>>> UDA.cpp?rev=305903&r1=305902&r2=305903&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/lib/Sema/SemaCUDA.cpp (original) >>>> +++ cfe/trunk/lib/Sema/SemaCUDA.cpp Wed Jun 21 07:46:57 2017 >>>> @@ -629,12 +629,6 @@ static bool IsKnownEmitted(Sema &S, Func >>>> // emitted, because (say) the definition could include "inline". >>>> FunctionDecl *Def = FD->getDefinition(); >>>> >>>> - // We may currently be parsing the body of FD, in which case >>>> - // FD->getDefinition() will be null, but we still want to treat FD >>>> as though >>>> - // it's a definition. >>>> - if (!Def && FD->willHaveBody()) >>>> - Def = FD; >>>> - >>>> if (Def && >>>> !isDiscardableGVALinkage(S.getASTContext().GetGVALinkageFor >>>> Function(Def))) >>>> return true; >>>> >>>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaD >>>> ecl.cpp?rev=305903&r1=305902&r2=305903&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) >>>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed Jun 21 07:46:57 2017 >>>> @@ -12232,6 +12232,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl >>>> >>>> if (FD) { >>>> FD->setBody(Body); >>>> + FD->setWillHaveBody(false); >>>> >>>> if (getLangOpts().CPlusPlus14) { >>>> if (!FD->isInvalidDecl() && Body && !FD->isDependentContext() && >>>> >>>> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaD >>>> eclCXX.cpp?rev=305903&r1=305902&r2=305903&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) >>>> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Wed Jun 21 07:46:57 2017 >>>> @@ -13878,6 +13878,9 @@ void Sema::SetDeclDeleted(Decl *Dcl, Sou >>>> return; >>>> } >>>> >>>> + // Deleted function does not have a body. >>>> + Fn->setWillHaveBody(false); >>>> + >>>> if (const FunctionDecl *Prev = Fn->getPreviousDecl()) { >>>> // Don't consider the implicit declaration we generate for explicit >>>> // specializations. FIXME: Do not generate these implicit >>>> declarations. >>>> >>>> Modified: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaT >>>> emplateInstantiateDecl.cpp?rev=305903&r1=305902&r2=305903&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp (original) >>>> +++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp Wed Jun 21 >>>> 07:46:57 2017 >>>> @@ -1782,6 +1782,9 @@ Decl *TemplateDeclInstantiator::VisitFun >>>> Previous.clear(); >>>> } >>>> >>>> + if (isFriend) >>>> + Function->setObjectOfFriendDecl(); >>>> + >>>> SemaRef.CheckFunctionDeclaration(/*Scope*/ nullptr, Function, >>>> Previous, >>>> isExplicitSpecialization); >>>> >>>> >>>> Modified: cfe/trunk/test/SemaCXX/friend2.cpp >>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/f >>>> riend2.cpp?rev=305903&r1=305902&r2=305903&view=diff >>>> ============================================================ >>>> ================== >>>> --- cfe/trunk/test/SemaCXX/friend2.cpp (original) >>>> +++ cfe/trunk/test/SemaCXX/friend2.cpp Wed Jun 21 07:46:57 2017 >>>> @@ -170,3 +170,40 @@ struct Test { >>>> template class Test<int>; >>>> >>>> } >>>> + >>>> +namespace pr14785 { >>>> +template<typename T> >>>> +struct Somewhat { >>>> + void internal() const { } >>>> + friend void operator+(int const &, Somewhat<T> const &) {} // >>>> expected-error{{redefinition of 'operator+'}} >>>> +}; >>>> + >>>> +void operator+(int const &, Somewhat<char> const &x) { // >>>> expected-note {{previous definition is here}} >>>> + x.internal(); // expected-note{{in instantiation of template class >>>> 'pr14785::Somewhat<char>' requested here}} >>>> +} >>>> +} >>>> + >>>> +namespace D30375 { >>>> +template <typename K> struct B { >>>> + template <typename A> bool insert(A &); >>>> +}; >>>> + >>>> +template <typename K> >>>> +template <typename A> bool B<K>::insert(A &x) { return x < x; } >>>> + >>>> +template <typename K> class D { >>>> + B<K> t; >>>> + >>>> +public: >>>> + K x; >>>> + bool insert() { return t.insert(x); } >>>> + template <typename K1> friend bool operator<(const D<K1> &, const >>>> D<K1> &); >>>> +}; >>>> + >>>> +template <typename K> bool operator<(const D<K> &, const D<K> &); >>>> + >>>> +void func() { >>>> + D<D<int>> cache; >>>> + cache.insert(); >>>> +} >>>> +} >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> cfe-commits@lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>> >>> >>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits