[clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[Arthur O'Dwyer via cfe-commits]

On Thu, Mar 26, 2020 at 11:49 PM Richard Smith <richard at metafoo.co.uk>
wrote:

> On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <arthur.j.odwyer at gmail.com>
>> wrote:
>>
>>> I'm not sure, but I do see that the call stack contains a call to
>>>
>>> bool llvm::function_ref<bool (clang::Expr*&, bool)>::callback_fn<llvm::function_ref<bool (clang::Expr*&, bool)> const>(long, clang::Expr*&, bool)
>>>
>>> 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.
>>> Here's function_ref's copy constructor:
>>>
>>>
>>>   template <typename Callable>
>>>
>>>   function_ref(Callable &&callable,
>>>
>>>
>>> std::enable_if_t<!std::is_same<std::remove_reference_t<Callable>,
>>>
>>>                                               function_ref>::value> * =
>>> nullptr)
>>>
>>>       : callback(callback_fn<typename
>>> std::remove_reference<Callable>::type>),
>>>
>>>         callable(reinterpret_cast<intptr_t>(&callable)) {}
>>>
>>>
>>> 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.
>>>
>>> Thoughts? [...]
>>>
>>
> 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.
>

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.
https://godbolt.org/z/oCvLpv
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`.
On Clang and EDG, [=], [i], and [i=i] all behave equivalently, and capture
a data member with type `I`.

I don't see anything about this on
https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=lambda%20capture%20const
— 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 bug 86697
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86697>.)

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.)

–Arthur
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200327/7f3b2533/attachment.html>