On Fri, Sep 21, 2012 at 2:58 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Fri, Sep 21, 2012 at 1:42 PM, Pan, Wei <span dir="ltr"><<a href="mailto:wei.pan@intel.com" target="_blank">wei.pan@intel.com</a>></span> wrote:<br></div><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">








<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hello Richard,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Does the attached patch look good for this bug? All your suggestions were applied.</span></p></div>


</div></blockquote><div><br></div></div><div>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.</div>
</div></blockquote><div><br></div><div>I've fixed this in r164437.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>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.</div>
<div><br></div><div>Do you need me to commit this for you?</div><div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><span style="color:rgb(31,73,125);font-family:Calibri,sans-serif;font-size:11pt"> </span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Wei<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a> [mailto:<a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a>]
<b>On Behalf Of </b>Richard Smith<br>
<b>Sent:</b> Friday, September 07, 2012 4:26 PM</span></p><div><div><br>
<b>To:</b> Pan, Wei<br>
<b>Cc:</b> <a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: [cfe-dev] Clang crash on infinite template instantiation<u></u><u></u></div></div><p></p>
</div><div><div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<div>
<p class="MsoNormal">> --- a/include/clang/Sema/Sema.h<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal">> +++ b/include/clang/Sema/Sema.h<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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@.<u></u><u></u></p>



</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">The patch generally looks fine. A few minor things:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">> @@ -1939,6 +1939,10 @@ public:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>      BEF_end<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>    };<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>  <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> +  /// IsBuildingRecoveryCallExpr - True if Sema is building a recovery call<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Please use \brief instead of repeating the variable name.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">> +  /// expression.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> +  bool IsBuildingRecoveryCallExpr;<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> +<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>    ForRangeStatus BuildForRangeBeginEndCall(Scope *S, SourceLocation Loc,<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Please move this to the top of the class, with the other member variables.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">> +  // template <typename T> auto foo(T t) -> decltpye(foo(t)) {}<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> +  // template <typename T> auto foo(T t) -> decltpye(foo(&t)) {}<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Typo "decltpye".<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<div>
<p class="MsoNormal">> +++ b/test/SemaTemplate/instantiation-depth-subst.cpp<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> @@ -1,9 +1,6 @@<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>  // RUN: %clang_cc1 -std=c++11 -verify %s -ftemplate-depth 2<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>  <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>  // PR9793<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> -template<typename T> auto f(T t) -> decltype(f(t)); // \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> -// expected-error {{recursive template instantiation exceeded maximum depth of 2}} \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> -// expected-note 3 {{while substituting}} \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> -// expected-note {{candidate}}<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> +template<typename T> auto f(T t) -> decltype(f(t)); // expected-note {{candidate template ignored}}<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">>  int k = f(0); // expected-error {{no matching function for call to 'f'}}<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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):<u></u><u></u></p>



</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">-int k = f(0);<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+struct S {};<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+int k = f(S());<u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Fri, Sep 7, 2012 at 9:39 AM, Pan, Wei <<a href="mailto:wei.pan@intel.com" target="_blank">wei.pan@intel.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hi Richard,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Disabling BuildRecoveryCallExpr seems correct to solving this problem. Do you think this patch is
 correct?</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks!</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Wei</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">
<a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a> [mailto:<a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a>]
<b>On Behalf Of </b>Richard Smith<br>
<b>Sent:</b> Thursday, September 06, 2012 6:26 PM<br>
<b>To:</b> Pan, Wei<br>
<b>Cc:</b> <a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: [cfe-dev] Clang crash on infinite template instantiation</span><u></u><u></u></p>
</div>
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">On Thu, Sep 6, 2012 at 1:50 PM, Pan, Wei <<a href="mailto:wei.pan@intel.com" target="_blank">wei.pan@intel.com</a>> wrote:<u></u><u></u></p>
<div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<p class="MsoNormal">Hello Clang Dev,<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Recently I looked into the clang bug 12053,<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">template <typename T> auto foo(T t) -> decltype(foo(t)) {}<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"><a href="http://llvm.org/bugs/show_bug.cgi?id=12053" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=12053</a><u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">which crashes clang (trunk)  (and gcc 4.6 too).<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">As far as I know,  Clang does realize that there is no candidate
<u></u><u></u></p>
<p class="MsoNormal">available while resolving “decltype(foo(t))”, however BuildRecoveryCallExpr
<u></u><u></u></p>
<p class="MsoNormal">will find  template foo (in DiagnoseTwoPhaseLookup)
<u></u><u></u></p>
<p class="MsoNormal">and try to instantiate it again. This leads to the crash.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">I am wondering if the following is the right way to fix this. The basic idea is:
<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Before starting instantiating a function template, we check if the same function<u></u><u></u></p>
<p class="MsoNormal">template instantiation *with the same template arguments* is already in-progress.
<u></u><u></u></p>
<p class="MsoNormal">If yes, then clang is not making any progress and should lead an infinite loop.<u></u><u></u></p>
<p class="MsoNormal">We treat it as an SFINAE.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">The attached patch will fix the clang crashing on the above test and other similar tests like<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">template <typename T> auto foo(T t) -> decltype(bar(t)) {}<u></u><u></u></p>
<p class="MsoNormal">template <typename T> auto bar(T t) -> decltype(foo(t)) {}<u></u><u></u></p>
<p class="MsoNormal">int x = foo(0);<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">This is not a final patch since this change will affect two tests (only these two)
<u></u><u></u></p>
<p class="MsoNormal">instantiation-depth-subst.cpp and instantiation-depth-subst-2.cpp.<u></u><u></u></p>
<p class="MsoNormal">Any thoughts?<u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Also, this approach doesn't solve the entire problem. For instance, in this case, there is no cycle:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  template<typename T> auto foo(T t) -> decltype(foo(&t)) {}<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Perhaps we can solve this more directly, by just disabling BuildRecoveryCallExpr when we're already in the middle of recovery.<u></u><u></u></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div></div></div>
</div>

</blockquote></div></div></div><br>
</blockquote></div><br>