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

[Arthur O'Dwyer via cfe-commits]

Richard: Okay, filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94376 !

David: You are correct, the bug in function_ref appeared only when
constructing from `const function_ref&&`. When I said that the constructor
template suppressed the copy constructor, I was wrong. The implicitly
generated copy constructor is still there, and it's the best match for
`const function_ref&` but not for `const function_ref&&`.  The bug would
also appear with construction from `volatile function_ref&`, but, let's not
go there. :)

IMHO (YMMV), it would be worth *additionally* adding a complex but
"realistic" test case that depends on the GCC bug, as opposed to your own
test's simple but "unrealistic" cast to `const function_ref&&`. Here's a
test case that I believe would have dereferenced null on GCC but would have
worked fine on Clang—

TEST(FunctionRefTest, BadCopyGCCBug) {

  auto A = [] { return 1; };

  function_ref<int()> W = A;

  auto LW = [=]() {  // captures a data member 'const function_ref W'

    return [=]() {

      return W();

    };

  }();

  auto LX = std::move(LW);  // calls function_ref's 'const&&' constructor

  W = nullptr;

  ASSERT_EQ(LX(), 1);

}

–Arthur


On Fri, Mar 27, 2020 at 7:48 PM Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, 26 Mar 2020 at 21:50, Arthur O'Dwyer via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> [...]
>> 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/0797f03e/attachment.html>