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

[Pan, Wei]

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] 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<mailto: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> [mailto: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<mailto: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<mailto: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/20120921/3b6185b3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr12053.patch
Type: application/octet-stream
Size: 4106 bytes
Desc: pr12053.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120921/3b6185b3/attachment.obj>