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

[David Blaikie via cfe-commits]

On Fri, Mar 27, 2020 at 5:16 PM Arthur O'Dwyer via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> 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);
>
> }
>

Yeah, I tend to err on the side of fairly isolated testing. I guess a
fairly closer-to-practical, though smaller (in terms of feature
interactions, not necessarily in lines of code) example might be to
std::move on a const ref, rather than the raw cast - the next step beyond
that would be to use a template to make it more realistic as to why you'd
be moving a const ref, etc.

But I think the narrower test is alright.

- Dave


>
> –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.
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200330/008e9926/attachment-0001.html>