On Fri, Sep 21, 2012 at 2:58 PM, Richard Smith <[email protected]>wrote:
> On Fri, Sep 21, 2012 at 1:42 PM, Pan, Wei <[email protected]> wrote: > >> Hello Richard,**** >> >> ** ** >> >> Does the attached patch look good for this bug? All your suggestions were >> applied. >> > > I'm surprised that we now produce four (fatal!) errors for > instantiation-depth-subst.cpp. We shouldn't be emitting diagnostics after > the first fatal error. Do you know why it's happening? Please add a FIXME > to that test for that. > I've fixed this in r164437. > Please merge your new test file (test/SemaCXX/PR12053.cpp) into > test/SemaCXX/trailing-return-0x.cpp (put the contents into a namespace > PR12053 in that file). Then this LGTM. > > Do you need me to commit this for you? > > >> >> >> Thanks,**** >> >> ** ** >> >> Wei**** >> >> ** ** >> >> *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Richard >> Smith >> *Sent:* Friday, September 07, 2012 4:26 PM >> >> *To:* Pan, Wei >> *Cc:* [email protected] >> *Subject:* Re: [cfe-dev] Clang crash on infinite template instantiation** >> ** >> >> ** ** >> >> > --- a/include/clang/Sema/Sema.h**** >> >> > +++ b/include/clang/Sema/Sema.h**** >> >> ** ** >> >> For your future consideration: Convention on these lists is to use -p0 >> patches instead of -p1 patches. You can configure git with '[diff] noprefix >> = true' for this. Also, patches should generally be cfe-commits@, not >> cfe-dev@.**** >> >> ** ** >> >> ** ** >> >> The patch generally looks fine. A few minor things:**** >> >> ** ** >> >> > @@ -1939,6 +1939,10 @@ public:**** >> >> > BEF_end**** >> >> > };**** >> >> > **** >> >> > + /// IsBuildingRecoveryCallExpr - True if Sema is building a recovery >> call**** >> >> ** ** >> >> Please use \brief instead of repeating the variable name.**** >> >> ** ** >> >> > + /// expression.**** >> >> > + bool IsBuildingRecoveryCallExpr;**** >> >> > +**** >> >> > ForRangeStatus BuildForRangeBeginEndCall(Scope *S, SourceLocation >> Loc,**** >> >> ** ** >> >> Please move this to the top of the class, with the other member variables. >> **** >> >> ** ** >> >> ** ** >> >> > + // template <typename T> auto foo(T t) -> decltpye(foo(t)) {}**** >> >> > + // template <typename T> auto foo(T t) -> decltpye(foo(&t)) {}**** >> >> ** ** >> >> Typo "decltpye".**** >> >> ** ** >> >> ** ** >> >> > +++ b/test/SemaTemplate/instantiation-depth-subst.cpp**** >> >> > @@ -1,9 +1,6 @@**** >> >> > // RUN: %clang_cc1 -std=c++11 -verify %s -ftemplate-depth 2**** >> >> > **** >> >> > // PR9793**** >> >> > -template<typename T> auto f(T t) -> decltype(f(t)); // \**** >> >> > -// expected-error {{recursive template instantiation exceeded maximum >> depth of 2}} \**** >> >> > -// expected-note 3 {{while substituting}} \**** >> >> > -// expected-note {{candidate}}**** >> >> > +template<typename T> auto f(T t) -> decltype(f(t)); // expected-note >> {{candidate template ignored}}**** >> >> > **** >> >> > int k = f(0); // expected-error {{no matching function for call to >> 'f'}}**** >> >> ** ** >> >> This test is no longer testing what it was intended to test (that we have >> a depth limit for pure substitution). Please instead change the test as >> follows (so 'f' can be found within its own return type via ADL):**** >> >> ** ** >> >> -int k = f(0);**** >> >> +struct S {};**** >> >> +int k = f(S());**** >> >> ** ** >> >> On Fri, Sep 7, 2012 at 9:39 AM, Pan, Wei <[email protected]> wrote:**** >> >> Hi Richard,**** >> >> **** >> >> Disabling BuildRecoveryCallExpr seems correct to solving this problem. Do >> you think this patch is correct?**** >> >> **** >> >> Thanks!**** >> >> **** >> >> Wei**** >> >> **** >> >> *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Richard >> Smith >> *Sent:* Thursday, September 06, 2012 6:26 PM >> *To:* Pan, Wei >> *Cc:* [email protected] >> *Subject:* Re: [cfe-dev] Clang crash on infinite template instantiation** >> ** >> >> **** >> >> On Thu, Sep 6, 2012 at 1:50 PM, Pan, Wei <[email protected]> wrote:**** >> >> Hello Clang Dev,**** >> >> **** >> >> Recently I looked into the clang bug 12053,**** >> >> **** >> >> template <typename T> auto foo(T t) -> decltype(foo(t)) {}**** >> >> **** >> >> http://llvm.org/bugs/show_bug.cgi?id=12053**** >> >> **** >> >> which crashes clang (trunk) (and gcc 4.6 too).**** >> >> **** >> >> As far as I know, Clang does realize that there is no candidate **** >> >> available while resolving “decltype(foo(t))”, however >> BuildRecoveryCallExpr **** >> >> will find template foo (in DiagnoseTwoPhaseLookup) **** >> >> and try to instantiate it again. This leads to the crash.**** >> >> **** >> >> I am wondering if the following is the right way to fix this. The basic >> idea is: **** >> >> **** >> >> Before starting instantiating a function template, we check if the same >> function**** >> >> template instantiation *with the same template arguments* is already >> in-progress. **** >> >> If yes, then clang is not making any progress and should lead an infinite >> loop.**** >> >> We treat it as an SFINAE.**** >> >> **** >> >> The attached patch will fix the clang crashing on the above test and >> other similar tests like**** >> >> **** >> >> template <typename T> auto foo(T t) -> decltype(bar(t)) {}**** >> >> template <typename T> auto bar(T t) -> decltype(foo(t)) {}**** >> >> int x = foo(0);**** >> >> **** >> >> This is not a final patch since this change will affect two tests (only >> these two) **** >> >> instantiation-depth-subst.cpp and instantiation-depth-subst-2.cpp.**** >> >> Any thoughts?**** >> >> **** >> >> I generally like the idea of checking for cyclic function template >> instantiations, but I'm hesitant about this approach -- scanning through >> the instantiation stack each time introduces overhead which grows >> quadratically in the instantiation depth, which could be too slow for deep >> instantiation stacks.**** >> >> **** >> >> Also, this approach doesn't solve the entire problem. For instance, in >> this case, there is no cycle:**** >> >> **** >> >> template<typename T> auto foo(T t) -> decltype(foo(&t)) {}**** >> >> **** >> >> Perhaps we can solve this more directly, by just disabling >> BuildRecoveryCallExpr when we're already in the middle of recovery.**** >> >> ** ** >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
