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

[Richard Smith via cfe-commits]

On Thu, 26 Mar 2020 at 21:50, Arthur O'Dwyer via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

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

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! :)

[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."

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.

Given that you identified the GCC capture bug, I think you should do the
honors :)


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

Yup.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200327/46e754be/attachment.html>