<div dir="ltr"><div dir="ltr">On Thu, 26 Mar 2020 at 21:50, Arthur O'Dwyer via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Thu, Mar 26, 2020 at 11:49 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>> wrote:<br></div><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">I'm not sure, but I do see that the call stack contains a call to<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"></span></p><pre style="color:rgb(0,0,0);font-family:"Courier New",courier,monotype,monospace"><span>bool llvm::function_ref<bool (clang::Expr*&, bool)>::callback_fn<llvm::function_ref<bool (clang::Expr*&, bool)> const>(long, clang::Expr*&, bool)<br></span></pre>Notice that this is function_ref::callback_fn<T> instantiated with T=function_ref itself; i.e., we do have a function_ref to a function_ref. This is the thing I was saying can happen (in my "lamb/sheep" example) but which I thought should never happen in this codepath.<br>Here's function_ref's copy constructor:<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p><p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> template <typename Callable></span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> function_ref(Callable &&callable,</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> std::enable_if_t<!std::is_same<std::remove_reference_t<Callable>,</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> function_ref>::value> * = nullptr)</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> : callback(callback_fn<typename std::remove_reference<Callable>::type>),</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> callable(reinterpret_cast<intptr_t>(&callable)) {}</span></p><p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p>I believe that it should be using std::remove_cvref_t, not std::remove_reference_t, so as not to conflict with the compiler's implicitly generated copy constructor.</div><div dir="ltr"><br>Thoughts? [...]</div></div></div></blockquote></div></div></div></div></blockquote><div><br></div><div>OK, so: we're calling the wrong constructor for the inner capture due to the implicit 'const' that's added because the outer lambda is not mutable (and the fix suggested by Arthur is the right one: we should be using remove_cvref_t here not remove_reference_t -- or rather std::remove_cv_t<std::remove_reference_t<Callable>>, since we don't require C++20 yet). And this means that copying a function_ref from a *const* function_ref gives you a new function_ref that refers to the old one, not a new function_ref that refers to the same function the old one did.</div></div></div></blockquote><div><br></div><div>Argh! That is insanely sneaky, and it is quite probable IMHO that this is a core-language bug in GCC, because GCC behaves differently from EDG and Clang here.</div><div></div><div><a href="https://godbolt.org/z/oCvLpv" target="_blank">https://godbolt.org/z/oCvLpv</a><br></div><div>On GCC, when you have a lambda nested within a lambda, and you capture-by-copy a variable which was captured-by-copy using [=] or [i] (but not C++14 init-capture [i=i]), then your data member for that capture will have type `const I`, not just `I`.</div><div>On Clang and EDG, [=], [i], and [i=i] all behave equivalently, and capture a data member with type `I`.</div><div><br></div><div>I don't see anything about this on <a href="https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=lambda%20capture%20const" target="_blank">https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=lambda%20capture%20const</a> — Richard, you might be best able to describe the issue correctly ;) but if you want me to file it, I will. (Might be a corollary of <a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86697" target="_blank">bug 86697</a>.)</div></div></div></div></div></div></div></div></blockquote><div><br></div><div>Oh wow, so this would only fail with a combination of libstdc++ (which makes a copy of the lambda and destroy the original before copying the lambda) and GCC (which has a bug in how it determines the type of a nested capture)? Fascinating! :)</div><div><br></div><div>[expr.prim.lambda.capture]p10 is the relevant rule: "The type of such a data member is the referenced type if the entity is a reference to an object, an lvalue reference to the referenced function type if the entity is a reference to a function, or the type of the corresponding captured entity otherwise."</div><div><br></div><div>Regardless of whether you think the captured entity is the original variable or the member of the outer closure type, the type of that entity is not const-qualified. So the inner capture should not have a const-qualified type.</div><div><br></div><div>Given that you identified the GCC capture bug, I think you should do the honors :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div>Regardless, llvm::function_ref's SFINAE should still be fixed. This is a bug in llvm::function_ref, exposed by a bug in GCC. (Or vice versa.)</div></div></div></div></div></div></div></div></blockquote><div><br></div><div>Yup. </div></div></div>