[cfe-commits] [cfe-dev] Clang crash on infinite template instantiation

[Richard Smith]

Thanks, I've checked this in as r164586.

On Mon, Sep 24, 2012 at 7:09 AM, Pan, Wei <wei.pan at intel.com> wrote:

>  Hi Richard,****
>
> ** **
>
> I just updated the patch to sync with some recent changes. Now no change
> will be made on the test instantiation-depth-subst.cpp.****
>
> ** **
>
> Thanks for reviewing!****
>
> ** **
>
> Wei****
>
> ** **
>
> *From:* Pan, Wei
> *Sent:* Friday, September 21, 2012 4:43 PM
> *To:* Richard Smith
> *Cc:* cfe-commits at cs.uiuc.edu
> *Subject:* RE: [cfe-dev] Clang crash on infinite template instantiation***
> *
>
> ** **
>
> Hello Richard,****
>
> ** **
>
> Does the attached patch look good for this bug? All your suggestions were
> applied.****
>
> ** **
>
> Thanks,****
>
> ** **
>
> Wei****
>
> ** **
>
> *From:* metafoo at gmail.com [mailto:metafoo at gmail.com <metafoo at gmail.com>] *On
> Behalf Of *Richard Smith
> *Sent:* Friday, September 07, 2012 4:26 PM
> *To:* Pan, Wei
> *Cc:* cfe-dev at cs.uiuc.edu
> *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 at .****
>
> ** **
>
> ** **
>
> 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 <wei.pan at intel.com> wrote:****
>
> Hi Richard,****
>
>  ****
>
> Disabling BuildRecoveryCallExpr seems correct to solving this problem. Do
> you think this patch is correct?****
>
>  ****
>
> Thanks!****
>
>  ****
>
> Wei****
>
>  ****
>
> *From:* metafoo at gmail.com [mailto:metafoo at gmail.com] *On Behalf Of *Richard
> Smith
> *Sent:* Thursday, September 06, 2012 6:26 PM
> *To:* Pan, Wei
> *Cc:* cfe-dev at cs.uiuc.edu
> *Subject:* Re: [cfe-dev] Clang crash on infinite template instantiation***
> *
>
>  ****
>
> On Thu, Sep 6, 2012 at 1:50 PM, Pan, Wei <wei.pan at intel.com> 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.****
>
> ** **
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120924/4ed9ea29/attachment.html>