Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Fri, Mar 27, 2020 at 5:16 PM Arthur O'Dwyer via cfe-commits <
cfe-commits@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 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
> wrote:
>
>> On Thu, 26 Mar 2020 at 21:50, Arthur O'Dwyer via cfe-commits <
>> cfe-commits@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
>>> .)
>>>
>>
>> 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@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Fri, Mar 27, 2020 at 8:16 PM Arthur O'Dwyer
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 W = A;
>
> auto LW = [=]() { // captures a data member 'const function_ref W'
>
>
...and naturally I put the comment on the wrong line. :P
The const-qualified capture happens on the *following* line.
> return [=]() {
>
> return W();
>
> };
>
> }();
>
> auto LX = std::move(LW); // calls function_ref's 'const&&' constructor
>
> W = nullptr;
>
> ASSERT_EQ(LX(), 1);
>
> }
>
–Arthur
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
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 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 wrote:
> On Thu, 26 Mar 2020 at 21:50, Arthur O'Dwyer via cfe-commits <
> cfe-commits@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
>> .)
>>
>
> 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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Fri, 27 Mar 2020 at 16:35, David Blaikie via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Thu, Mar 26, 2020 at 8:49 PM Richard Smith
> wrote:
>
>> On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <
>>> arthur.j.odw...@gmail.com> wrote:
>>>
I'm not sure, but I do see that the call stack contains a call to
bool llvm::function_ref>>> bool)>::callback_fn
const>(long, clang::Expr*&, bool)
Notice that this is function_ref::callback_fn 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
function_ref(Callable &&callable,
std::enable_if_t,
function_ref>::value> * =
nullptr)
: callback(callback_fn>>> std::remove_reference::type>),
callable(reinterpret_cast(&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?
>>>
>>> Yep, looks like you're on to something here - I still don't understand
>>> why calling that function rather than the real trivial copy ctor would be
>>> problematic,
>>>
>>
>> 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>, 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. In
>> particular, this happens when copying a lambda that captures a function_ref
>> by value.
>>
>
> I was only able to hit this with a const rvalue reference - is that
> expected, or did I overcomplicate my test case in some way?
>
No, my analysis was incomplete, sorry. For a const function_ref&, we call
the implicit copy constructor rather than the constructor template. const
function_ref&& breaks because it isn't an exact match for an implicit
copy/move constructor.
> (this: "const function_ref a; function_ref b = a;" didn't hit the
> converting ctor, it ran the copy ctor as desired. I had to "function_ref
> b = static_cast &&>(a);" to tickle it)
>
> Committed the fix/test in cbce88dd3a9ea7161da3c57749cf03873dc7ea79 - open
> to suggestions on simplification of the test case if there is some. Oh, I
> might've named the new test case a bit poorly... didn't go back and edit
> that after it got more baked.
>
>
>>
>> The next part of the puzzle is that llvm::any_of takes its callback by
>> value and passes it by value to std::any_of. And inside any_of, in
>> libstdc++, the predicate is passed through __pred_iter (
>> https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/predefined_ops.h#L323)
>> before being used. That results in building a function_ref referring to
>> part of the function parameter in __pred_iter, whose lifetime ends before
>> we call it.
>>
>> With libc++, the predicate is not copied around inside any_of, which is
>> why this was only failing on some buildbots.
>>
>> but I managed to reproduce this locally with GCC 7.5 (seems to be an
>>> issue with the 7 series - the buildbot used 7.3) & if I capture by value in
>>> both outer and inner lambdas it reproduces, but if I mark the outer lambda
>>> as mutable it succeeds (because this would remove the const & not trip over
>>> the SFINAE issue you pointed out)...
>>>
>>> Investigation continues.
>>>
>>> - Dave
>>>
>>>
–Arthur
On Thu, Mar 26, 2020 at 5:08 PM David Blaikie
wrote:
> Hey Richard - could you help try to diagnose what's happening here?
>
> I reverted this patch in:
> https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb
> But that did actually cause buildbot failures, such as these ones:
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491
> eg:
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adeclare_variant_mixed_codegen.cpp
> I "fixed" these failures blind by reapplying part of this original
> commit (the lambda capture by reference rather than by value):
> https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9
>
> I've stared at this a fair bit
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Thu, 26 Mar 2020 at 21:50, Arthur O'Dwyer via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Thu, Mar 26, 2020 at 11:49 PM Richard Smith
> wrote:
>
>> On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <
>>> arthur.j.odw...@gmail.com> wrote:
>>>
I'm not sure, but I do see that the call stack contains a call to
bool llvm::function_ref>>> bool)>::callback_fn
const>(long, clang::Expr*&, bool)
Notice that this is function_ref::callback_fn 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
function_ref(Callable &&callable,
std::enable_if_t,
function_ref>::value> * =
nullptr)
: callback(callback_fn>>> std::remove_reference::type>),
callable(reinterpret_cast(&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>, 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
> .)
>
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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Thu, Mar 26, 2020 at 8:49 PM Richard Smith wrote:
> On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer
>> wrote:
>>
>>> I'm not sure, but I do see that the call stack contains a call to
>>>
>>> bool llvm::function_ref>> bool)>::callback_fn
>>> const>(long, clang::Expr*&, bool)
>>>
>>> Notice that this is function_ref::callback_fn 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
>>>
>>> function_ref(Callable &&callable,
>>>
>>>
>>> std::enable_if_t,
>>>
>>> function_ref>::value> * =
>>> nullptr)
>>>
>>> : callback(callback_fn>> std::remove_reference::type>),
>>>
>>> callable(reinterpret_cast(&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?
>>>
>>
>> Yep, looks like you're on to something here - I still don't understand
>> why calling that function rather than the real trivial copy ctor would be
>> problematic,
>>
>
> 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>, 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. In
> particular, this happens when copying a lambda that captures a function_ref
> by value.
>
I was only able to hit this with a const rvalue reference - is that
expected, or did I overcomplicate my test case in some way? (this: "const
function_ref a; function_ref b = a;" didn't hit the converting ctor,
it ran the copy ctor as desired. I had to "function_ref b =
static_cast &&>(a);" to tickle it)
Committed the fix/test in cbce88dd3a9ea7161da3c57749cf03873dc7ea79 - open
to suggestions on simplification of the test case if there is some. Oh, I
might've named the new test case a bit poorly... didn't go back and edit
that after it got more baked.
>
> The next part of the puzzle is that llvm::any_of takes its callback by
> value and passes it by value to std::any_of. And inside any_of, in
> libstdc++, the predicate is passed through __pred_iter (
> https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/predefined_ops.h#L323)
> before being used. That results in building a function_ref referring to
> part of the function parameter in __pred_iter, whose lifetime ends before
> we call it.
>
> With libc++, the predicate is not copied around inside any_of, which is
> why this was only failing on some buildbots.
>
> but I managed to reproduce this locally with GCC 7.5 (seems to be an issue
>> with the 7 series - the buildbot used 7.3) & if I capture by value in both
>> outer and inner lambdas it reproduces, but if I mark the outer lambda as
>> mutable it succeeds (because this would remove the const & not trip over
>> the SFINAE issue you pointed out)...
>>
>> Investigation continues.
>>
>> - Dave
>>
>>
>>> –Arthur
>>>
>>>
>>>
>>> On Thu, Mar 26, 2020 at 5:08 PM David Blaikie
>>> wrote:
>>>
Hey Richard - could you help try to diagnose what's happening here?
I reverted this patch in:
https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb
But that did actually cause buildbot failures, such as these ones:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491
eg:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adeclare_variant_mixed_codegen.cpp
I "fixed" these failures blind by reapplying part of this original
commit (the lambda capture by reference rather than by value):
https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9
I've stared at this a fair bit and can't spot any undefined behavior,
but I guess it's probably in there somewhere - and I'm worried that this
fix is blind, not fully justified & might be hiding some latent issues.
The full buildbot example failure quoted here in case it times out/gets
deleted on the buildbot itself:
TEST 'Clang ::
OpenMP/declare_variant_mixed_codegen.cpp' FAILED
Script:
--
: 'RUN: at line 1';
/home/buildbots
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Thu, Mar 26, 2020 at 11:49 PM Richard Smith
wrote:
> On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer
>> wrote:
>>
>>> I'm not sure, but I do see that the call stack contains a call to
>>>
>>> bool llvm::function_ref>> bool)>::callback_fn
>>> const>(long, clang::Expr*&, bool)
>>>
>>> Notice that this is function_ref::callback_fn 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
>>>
>>> function_ref(Callable &&callable,
>>>
>>>
>>> std::enable_if_t,
>>>
>>> function_ref>::value> * =
>>> nullptr)
>>>
>>> : callback(callback_fn>> std::remove_reference::type>),
>>>
>>> callable(reinterpret_cast(&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>, 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
.)
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer
> wrote:
>
>> I'm not sure, but I do see that the call stack contains a call to
>>
>> bool llvm::function_ref> bool)>::callback_fn
>> const>(long, clang::Expr*&, bool)
>>
>> Notice that this is function_ref::callback_fn 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
>>
>> function_ref(Callable &&callable,
>>
>>
>> std::enable_if_t,
>>
>> function_ref>::value> * =
>> nullptr)
>>
>> : callback(callback_fn> std::remove_reference::type>),
>>
>> callable(reinterpret_cast(&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?
>>
>
> Yep, looks like you're on to something here - I still don't understand why
> calling that function rather than the real trivial copy ctor would be
> problematic,
>
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>, 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. In
particular, this happens when copying a lambda that captures a function_ref
by value.
The next part of the puzzle is that llvm::any_of takes its callback by
value and passes it by value to std::any_of. And inside any_of, in
libstdc++, the predicate is passed through __pred_iter (
https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/predefined_ops.h#L323)
before being used. That results in building a function_ref referring to
part of the function parameter in __pred_iter, whose lifetime ends before
we call it.
With libc++, the predicate is not copied around inside any_of, which is why
this was only failing on some buildbots.
but I managed to reproduce this locally with GCC 7.5 (seems to be an issue
> with the 7 series - the buildbot used 7.3) & if I capture by value in both
> outer and inner lambdas it reproduces, but if I mark the outer lambda as
> mutable it succeeds (because this would remove the const & not trip over
> the SFINAE issue you pointed out)...
>
> Investigation continues.
>
> - Dave
>
>
>> –Arthur
>>
>>
>>
>> On Thu, Mar 26, 2020 at 5:08 PM David Blaikie wrote:
>>
>>> Hey Richard - could you help try to diagnose what's happening here?
>>>
>>> I reverted this patch in:
>>> https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb
>>> But that did actually cause buildbot failures, such as these ones:
>>> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491
>>> eg:
>>> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adeclare_variant_mixed_codegen.cpp
>>> I "fixed" these failures blind by reapplying part of this original
>>> commit (the lambda capture by reference rather than by value):
>>> https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9
>>>
>>> I've stared at this a fair bit and can't spot any undefined behavior,
>>> but I guess it's probably in there somewhere - and I'm worried that this
>>> fix is blind, not fully justified & might be hiding some latent issues.
>>>
>>> The full buildbot example failure quoted here in case it times out/gets
>>> deleted on the buildbot itself:
>>>
>>> TEST 'Clang ::
>>> OpenMP/declare_variant_mixed_codegen.cpp' FAILED
>>> Script:
>>> --
>>> : 'RUN: at line 1';
>>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
>>> -cc1 -internal-isystem
>>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
>>> -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-unknown-linux
>>> -emit-llvm
>>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
>>> -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope |
>>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/FileCheck
>>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
>>> : 'RUN: at line 2';
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer
wrote:
> I'm not sure, but I do see that the call stack contains a call to
>
> bool llvm::function_ref bool)>::callback_fn
> const>(long, clang::Expr*&, bool)
>
> Notice that this is function_ref::callback_fn 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
>
> function_ref(Callable &&callable,
>
>
> std::enable_if_t,
>
> function_ref>::value> * =
> nullptr)
>
> : callback(callback_fn std::remove_reference::type>),
>
> callable(reinterpret_cast(&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?
>
Yep, looks like you're on to something here - I still don't understand why
calling that function rather than the real trivial copy ctor would be
problematic, but I managed to reproduce this locally with GCC 7.5 (seems to
be an issue with the 7 series - the buildbot used 7.3) & if I capture by
value in both outer and inner lambdas it reproduces, but if I mark the
outer lambda as mutable it succeeds (because this would remove the const &
not trip over the SFINAE issue you pointed out)...
Investigation continues.
- Dave
> –Arthur
>
>
>
> On Thu, Mar 26, 2020 at 5:08 PM David Blaikie wrote:
>
>> Hey Richard - could you help try to diagnose what's happening here?
>>
>> I reverted this patch in:
>> https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb
>> But that did actually cause buildbot failures, such as these ones:
>> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491
>> eg:
>> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adeclare_variant_mixed_codegen.cpp
>> I "fixed" these failures blind by reapplying part of this original commit
>> (the lambda capture by reference rather than by value):
>> https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9
>>
>> I've stared at this a fair bit and can't spot any undefined behavior, but
>> I guess it's probably in there somewhere - and I'm worried that this fix is
>> blind, not fully justified & might be hiding some latent issues.
>>
>> The full buildbot example failure quoted here in case it times out/gets
>> deleted on the buildbot itself:
>>
>> TEST 'Clang ::
>> OpenMP/declare_variant_mixed_codegen.cpp' FAILED
>> Script:
>> --
>> : 'RUN: at line 1';
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
>> -cc1 -internal-isystem
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
>> -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-unknown-linux
>> -emit-llvm
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
>> -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope |
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/FileCheck
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
>> : 'RUN: at line 2';
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
>> -cc1 -internal-isystem
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
>> -nostdsysteminc -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-linux
>> -fexceptions -fcxx-exceptions -emit-pch -o
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/tools/clang/test/OpenMP/Output/declare_variant_mixed_codegen.cpp.tmp
>> -fopenmp-version=50
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
>> : 'RUN: at line 3';
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
>> -cc1 -internal-isystem
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
>> -nostdsysteminc -fopenmp -x c++ -triple x86_64-unknown-linux -fexceptions
>> -fcxx-exceptions -std=c++11 -include-pch
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/tools/clang/test/OpenMP/Output/declare_variant_mixed_codegen.cpp.tmp
>> -verify
>> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
>> -emit-llvm -o - -fopenmp-version=50 |
>> /home/buildbots/ppc64be-clang-multistage-t
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
I'm not sure, but I do see that the call stack contains a call to
bool llvm::function_ref::callback_fn
const>(long, clang::Expr*&, bool)
Notice that this is function_ref::callback_fn 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
function_ref(Callable &&callable,
std::enable_if_t,
function_ref>::value> * =
nullptr)
: callback(callback_fn::type>),
callable(reinterpret_cast(&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?
–Arthur
On Thu, Mar 26, 2020 at 5:08 PM David Blaikie wrote:
> Hey Richard - could you help try to diagnose what's happening here?
>
> I reverted this patch in:
> https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb
> But that did actually cause buildbot failures, such as these ones:
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491
> eg:
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adeclare_variant_mixed_codegen.cpp
> I "fixed" these failures blind by reapplying part of this original commit
> (the lambda capture by reference rather than by value):
> https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9
>
> I've stared at this a fair bit and can't spot any undefined behavior, but
> I guess it's probably in there somewhere - and I'm worried that this fix is
> blind, not fully justified & might be hiding some latent issues.
>
> The full buildbot example failure quoted here in case it times out/gets
> deleted on the buildbot itself:
>
> TEST 'Clang ::
> OpenMP/declare_variant_mixed_codegen.cpp' FAILED
> Script:
> --
> : 'RUN: at line 1';
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
> -cc1 -internal-isystem
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
> -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-unknown-linux
> -emit-llvm
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
> -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope |
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/FileCheck
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
> : 'RUN: at line 2';
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
> -cc1 -internal-isystem
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
> -nostdsysteminc -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-linux
> -fexceptions -fcxx-exceptions -emit-pch -o
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/tools/clang/test/OpenMP/Output/declare_variant_mixed_codegen.cpp.tmp
> -fopenmp-version=50
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
> : 'RUN: at line 3';
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
> -cc1 -internal-isystem
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
> -nostdsysteminc -fopenmp -x c++ -triple x86_64-unknown-linux -fexceptions
> -fcxx-exceptions -std=c++11 -include-pch
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/tools/clang/test/OpenMP/Output/declare_variant_mixed_codegen.cpp.tmp
> -verify
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
> -emit-llvm -o - -fopenmp-version=50 |
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/FileCheck
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
> --
> Exit Code: 2
>
> Command Output (stderr):
> --
> Stack dump:
> 0. Program arguments:
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang
> -cc1 -internal-isystem
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include
> -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-unknown-linux
> -emit-llvm
> /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp
> -fexceptions -fcxx-e
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
Hey Richard - could you help try to diagnose what's happening here? I reverted this patch in: https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb But that did actually cause buildbot failures, such as these ones: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491 eg: http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adeclare_variant_mixed_codegen.cpp I "fixed" these failures blind by reapplying part of this original commit (the lambda capture by reference rather than by value): https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9 I've stared at this a fair bit and can't spot any undefined behavior, but I guess it's probably in there somewhere - and I'm worried that this fix is blind, not fully justified & might be hiding some latent issues. The full buildbot example failure quoted here in case it times out/gets deleted on the buildbot itself: TEST 'Clang :: OpenMP/declare_variant_mixed_codegen.cpp' FAILED Script: -- : 'RUN: at line 1'; /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang -cc1 -internal-isystem /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-unknown-linux -emit-llvm /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope | /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/FileCheck /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp : 'RUN: at line 2'; /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang -cc1 -internal-isystem /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include -nostdsysteminc -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-linux -fexceptions -fcxx-exceptions -emit-pch -o /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/tools/clang/test/OpenMP/Output/declare_variant_mixed_codegen.cpp.tmp -fopenmp-version=50 /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp : 'RUN: at line 3'; /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang -cc1 -internal-isystem /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include -nostdsysteminc -fopenmp -x c++ -triple x86_64-unknown-linux -fexceptions -fcxx-exceptions -std=c++11 -include-pch /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/tools/clang/test/OpenMP/Output/declare_variant_mixed_codegen.cpp.tmp -verify /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp -emit-llvm -o - -fopenmp-version=50 | /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/FileCheck /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp -- Exit Code: 2 Command Output (stderr): -- Stack dump: 0. Program arguments: /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang -cc1 -internal-isystem /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/lib/clang/11.0.0/include -nostdsysteminc -verify -fopenmp -x c++ -triple x86_64-unknown-linux -emit-llvm /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope 1. /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp:38:92: at annotation token #0 0x12e3ebd0 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3ebd0) #1 0x12e3ece8 PrintStackTraceSignalHandler(void*) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3ece8) #2 0x12e3c4e0 llvm::sys::RunSignalHandlers() (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3c4e0) #3 0x12e3d3a4 SignalHandler(int) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3d3a4) #4 0x7fffb50f04d8 (linux-vdso64.so.1+0x4d8) #5 0x14df41ac bool llvm::function_ref::callback_fn const>(long, clang::Expr*&, bool) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
Apologies for the confusion.
I wrote the commit message after looking into this and I though the
issue was related to the capture by copy in the inner llvm::any_of and
the reuse in the outer. Looking back at the code I cannot say anymore
how I got that impression.
If you think the reference is problematic, I'm totally happy to remove
it. If the windows bots (or any other ones) don't like it we need to
investigate why. As mentioned, I had a problem recreating the problem
locally before.
Cheers,
Johannes
On 3/22/20 1:37 PM, Arthur O'Dwyer wrote:
> On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert
>> wrote:
>>
>>> Some buildbots, I think only Windows buildbots for some reason, crashed
>>> in this function.
>>>
>>> The reason, as described, is that an `llvm::function_ref` cannot be
>>> copied and then reused. It just happened to work on almost all
>>> configurations.
>>>
>>
>> That doesn't seem to be accurate, or if it is there's a lot of
mistakes in
>> the codebase - basically every function_ref parameter I can see in
LLVM is
>> passing by value, not by const ref. The implementation of
>> llvm::function_ref looks quite copyable so long as it doesn't
outlive the
>> functor it is pointing to.
>>
>
> David is correct. llvm::function_ref, like std::reference_wrapper, is a
> trivially copyable type, and it's designed to be copied.
> Like string_view and reference_wrapper, function_ref is designed to be
> passed by value. Redundantly passing function_ref *again by
reference* is a
> code smell.
>
> I've also checked the code here, and it looks like there are only two
> callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and they
> are both fine. FWIW, I would recommend reverting Johannes' change and
> seeing if those Windows buildbots are still unhappy (and if so, why).
>
>
> Btw, one failure mode I'm aware of, but which I believe is NOT
relevant in
> Johannes' case, is that `function_ref`'s converting constructor behaves
> differently from its copy constructor.
>
> int main()
>
> {
>
> auto lamb = [](){ return 42; };
>
> auto sheep = [](){ return 43; };
>
> llvm::function_ref one = lamb;
>
>
> llvm::function_ref twoA = one; // twoA refers to lamb
>
> llvm::function_ref twoB = one; // twoB refers to one which
> refers to lamb
>
>
> one = sheep;
>
>
> assert(twoA() == 42); // twoA refers to lamb
>
> assert(twoB() == 43); // twoB refers to one which refers to sheep
>
> }
>
> That is, if you have a function that takes a parameter of type
> function_ref, and someone passes it an argument of type
function_ref,
> then inside the function your parameter will be referring to that
argument
> itself instead of to its referent.
> However, in Johannes' particular case, we have no function_refs referring
> to other function_refs. We just make a lambda and take a function_ref to
> it, the end. So I'm pretty sure this pitfall is irrelevant.
>
> –Arthur
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what
happens with the buildbots.
On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert
wrote:
>
> Apologies for the confusion.
>
> I wrote the commit message after looking into this and I though the
> issue was related to the capture by copy in the inner llvm::any_of and
> the reuse in the outer. Looking back at the code I cannot say anymore
> how I got that impression.
>
> If you think the reference is problematic, I'm totally happy to remove
> it. If the windows bots (or any other ones) don't like it we need to
> investigate why. As mentioned, I had a problem recreating the problem
> locally before.
>
> Cheers,
>Johannes
>
>
> On 3/22/20 1:37 PM, Arthur O'Dwyer wrote:
> > On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <
> > cfe-commits@lists.llvm.org> wrote:
> >
> >> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert
>
> >> wrote:
> >>
> >>> Some buildbots, I think only Windows buildbots for some reason,
> crashed
> >>> in this function.
> >>>
> >>> The reason, as described, is that an `llvm::function_ref` cannot be
> >>> copied and then reused. It just happened to work on almost all
> >>> configurations.
> >>>
> >>
> >> That doesn't seem to be accurate, or if it is there's a lot of
> mistakes in
> >> the codebase - basically every function_ref parameter I can see in
> LLVM is
> >> passing by value, not by const ref. The implementation of
> >> llvm::function_ref looks quite copyable so long as it doesn't
> outlive the
> >> functor it is pointing to.
> >>
> >
> > David is correct. llvm::function_ref, like std::reference_wrapper, is a
> > trivially copyable type, and it's designed to be copied.
> > Like string_view and reference_wrapper, function_ref is designed to be
> > passed by value. Redundantly passing function_ref *again by
> reference* is a
> > code smell.
> >
> > I've also checked the code here, and it looks like there are only two
> > callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and
> they
> > are both fine. FWIW, I would recommend reverting Johannes' change and
> > seeing if those Windows buildbots are still unhappy (and if so, why).
> >
> >
> > Btw, one failure mode I'm aware of, but which I believe is NOT
> relevant in
> > Johannes' case, is that `function_ref`'s converting constructor behaves
> > differently from its copy constructor.
> >
> > int main()
> >
> > {
> >
> > auto lamb = [](){ return 42; };
> >
> > auto sheep = [](){ return 43; };
> >
> > llvm::function_ref one = lamb;
> >
> >
> > llvm::function_ref twoA = one;// twoA refers to lamb
> >
> > llvm::function_ref twoB = one; // twoB refers to one which
> > refers to lamb
> >
> >
> > one = sheep;
> >
> >
> > assert(twoA() == 42); // twoA refers to lamb
> >
> > assert(twoB() == 43); // twoB refers to one which refers to sheep
> >
> > }
> >
> > That is, if you have a function that takes a parameter of type
> > function_ref, and someone passes it an argument of type
> function_ref,
> > then inside the function your parameter will be referring to that
> argument
> > itself instead of to its referent.
> > However, in Johannes' particular case, we have no function_refs
> referring
> > to other function_refs. We just make a lambda and take a function_ref to
> > it, the end. So I'm pretty sure this pitfall is irrelevant.
> >
> > –Arthur
> >
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert
> wrote:
>
>> Some buildbots, I think only Windows buildbots for some reason, crashed
>> in this function.
>>
>> The reason, as described, is that an `llvm::function_ref` cannot be
>> copied and then reused. It just happened to work on almost all
>> configurations.
>>
>
> That doesn't seem to be accurate, or if it is there's a lot of mistakes in
> the codebase - basically every function_ref parameter I can see in LLVM is
> passing by value, not by const ref. The implementation of
> llvm::function_ref looks quite copyable so long as it doesn't outlive the
> functor it is pointing to.
>
David is correct. llvm::function_ref, like std::reference_wrapper, is a
trivially copyable type, and it's designed to be copied.
Like string_view and reference_wrapper, function_ref is designed to be
passed by value. Redundantly passing function_ref *again by reference* is a
code smell.
I've also checked the code here, and it looks like there are only two
callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and they
are both fine. FWIW, I would recommend reverting Johannes' change and
seeing if those Windows buildbots are still unhappy (and if so, why).
Btw, one failure mode I'm aware of, but which I believe is NOT relevant in
Johannes' case, is that `function_ref`'s converting constructor behaves
differently from its copy constructor.
int main()
{
auto lamb = [](){ return 42; };
auto sheep = [](){ return 43; };
llvm::function_ref one = lamb;
llvm::function_ref twoA = one;// twoA refers to lamb
llvm::function_ref twoB = one; // twoB refers to one which
refers to lamb
one = sheep;
assert(twoA() == 42); // twoA refers to lamb
assert(twoB() == 43); // twoB refers to one which refers to sheep
}
That is, if you have a function that takes a parameter of type
function_ref, and someone passes it an argument of type function_ref,
then inside the function your parameter will be referring to that argument
itself instead of to its referent.
However, in Johannes' particular case, we have no function_refs referring
to other function_refs. We just make a lambda and take a function_ref to
it, the end. So I'm pretty sure this pitfall is irrelevant.
–Arthur
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
Some buildbots, I think only Windows buildbots for some reason, crashed
in this function.
The reason, as described, is that an `llvm::function_ref` cannot be
copied and then reused. It just happened to work on almost all
configurations.
The change was not a "shot in the dark" but identified as proper fix by
examining the `llvm::function_ref` implementation.
I could not reproduce the error on any machine I had immediately access
to so maybe you can call that part "shot in the dark".
I'm not sure what else you want me to add here as a explanation but
please let me know if there is anything I can do.
On 3/22/20 12:21 AM, David Blaikie wrote:
"a problem" seems a smidge vague - was there a specific explanation for the
kind of problem that was signalled? Or was this fix a bit of a shot in the
dark?
On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
Author: Johannes Doerfert
Date: 2020-02-16T00:51:11-06:00
New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2
URL:
https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2
DIFF:
https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff
LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused
Some buildbots signaled a problem in this method when the
llvm::function_ref was copied and reused after 1228d42ddab8. To
eliminate the problem we avoid copying the llvm::function_ref and
instead we pass it as a const reference.
Added:
Modified:
clang/include/clang/AST/OpenMPClause.h
Removed:
diff --git a/clang/include/clang/AST/OpenMPClause.h
b/clang/include/clang/AST/OpenMPClause.h
index a3831fd5950f..453c068bbeb0 100644
--- a/clang/include/clang/AST/OpenMPClause.h
+++ b/clang/include/clang/AST/OpenMPClause.h
@@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
llvm::SmallVector Sets;
bool anyScoreOrCondition(
- llvm::function_ref Cond) {
-return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet &Set) {
+ const llvm::function_ref &Cond) {
+return llvm::any_of(Sets, [&Cond](OMPTraitInfo::OMPTraitSet &Set) {
return llvm::any_of(
- Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector &Selector)
{
+ Set.Selectors, [&Cond](OMPTraitInfo::OMPTraitSelector
&Selector) {
return Cond(Selector.ScoreOrCondition,
/* IsScore */ Selector.Kind !=
llvm::omp::TraitSelector::user_condition);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert
wrote:
> Some buildbots, I think only Windows buildbots for some reason, crashed
> in this function.
>
> The reason, as described, is that an `llvm::function_ref` cannot be
> copied and then reused. It just happened to work on almost all
> configurations.
>
That doesn't seem to be accurate, or if it is there's a lot of mistakes in
the codebase - basically every function_ref parameter I can see in LLVM is
passing by value, not by const ref. The implementation of
llvm::function_ref looks quite copyable so long as it doesn't outlive the
functor it is pointing to.
> The change was not a "shot in the dark" but identified as proper fix by
> examining the `llvm::function_ref` implementation.
>
What aspect of the implementation of llvm::function_ref makes it not safe
to copy in the way the code original did so? My reading of it makes it look
like it's OK to copy so long as it doesn't outlive the functor it is
pointing to (& the code as originall written doesn't look like it's
outliving the caller's functor)
> I could not reproduce the error on any machine I had immediately access
> to so maybe you can call that part "shot in the dark".
>
Sounds like this sort of thing could/should've shown up with msan, maybe?
Were you able to test with any sanitizers?
>
>
> I'm not sure what else you want me to add here as a explanation but
> please let me know if there is anything I can do.
>
>
> On 3/22/20 12:21 AM, David Blaikie wrote:
> > "a problem" seems a smidge vague - was there a specific explanation for
> the
> > kind of problem that was signalled? Or was this fix a bit of a shot in
> the
> > dark?
> >
> > On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <
> > cfe-commits@lists.llvm.org> wrote:
> >
> >> Author: Johannes Doerfert
> >> Date: 2020-02-16T00:51:11-06:00
> >> New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> >>
> >> URL:
> >>
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> >> DIFF:
> >>
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff
> >>
> >> LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused
> >>
> >> Some buildbots signaled a problem in this method when the
> >> llvm::function_ref was copied and reused after 1228d42ddab8. To
> >> eliminate the problem we avoid copying the llvm::function_ref and
> >> instead we pass it as a const reference.
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >> clang/include/clang/AST/OpenMPClause.h
> >>
> >> Removed:
> >>
> >>
> >>
> >>
> >>
>
> >> diff --git a/clang/include/clang/AST/OpenMPClause.h
> >> b/clang/include/clang/AST/OpenMPClause.h
> >> index a3831fd5950f..453c068bbeb0 100644
> >> --- a/clang/include/clang/AST/OpenMPClause.h
> >> +++ b/clang/include/clang/AST/OpenMPClause.h
> >> @@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
> >> llvm::SmallVector Sets;
> >>
> >> bool anyScoreOrCondition(
> >> - llvm::function_ref Cond) {
> >> -return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet &Set) {
> >> + const llvm::function_ref
> &Cond) {
> >> +return llvm::any_of(Sets, [&Cond](OMPTraitInfo::OMPTraitSet &Set) {
> >> return llvm::any_of(
> >> - Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector
> &Selector)
> >> {
> >> + Set.Selectors, [&Cond](OMPTraitInfo::OMPTraitSelector
> >> &Selector) {
> >> return Cond(Selector.ScoreOrCondition,
> >> /* IsScore */ Selector.Kind !=
> >> llvm::omp::TraitSelector::user_condition);
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
"a problem" seems a smidge vague - was there a specific explanation for the
kind of problem that was signalled? Or was this fix a bit of a shot in the
dark?
On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
>
> Author: Johannes Doerfert
> Date: 2020-02-16T00:51:11-06:00
> New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2
>
> URL:
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> DIFF:
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff
>
> LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused
>
> Some buildbots signaled a problem in this method when the
> llvm::function_ref was copied and reused after 1228d42ddab8. To
> eliminate the problem we avoid copying the llvm::function_ref and
> instead we pass it as a const reference.
>
> Added:
>
>
> Modified:
> clang/include/clang/AST/OpenMPClause.h
>
> Removed:
>
>
>
>
>
> diff --git a/clang/include/clang/AST/OpenMPClause.h
> b/clang/include/clang/AST/OpenMPClause.h
> index a3831fd5950f..453c068bbeb0 100644
> --- a/clang/include/clang/AST/OpenMPClause.h
> +++ b/clang/include/clang/AST/OpenMPClause.h
> @@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
>llvm::SmallVector Sets;
>
>bool anyScoreOrCondition(
> - llvm::function_ref Cond) {
> -return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet &Set) {
> + const llvm::function_ref &Cond) {
> +return llvm::any_of(Sets, [&Cond](OMPTraitInfo::OMPTraitSet &Set) {
>return llvm::any_of(
> - Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector &Selector)
> {
> + Set.Selectors, [&Cond](OMPTraitInfo::OMPTraitSelector
> &Selector) {
> return Cond(Selector.ScoreOrCondition,
> /* IsScore */ Selector.Kind !=
> llvm::omp::TraitSelector::user_condition);
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
Author: Johannes Doerfert
Date: 2020-02-16T00:51:11-06:00
New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2
URL:
https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2
DIFF:
https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff
LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused
Some buildbots signaled a problem in this method when the
llvm::function_ref was copied and reused after 1228d42ddab8. To
eliminate the problem we avoid copying the llvm::function_ref and
instead we pass it as a const reference.
Added:
Modified:
clang/include/clang/AST/OpenMPClause.h
Removed:
diff --git a/clang/include/clang/AST/OpenMPClause.h
b/clang/include/clang/AST/OpenMPClause.h
index a3831fd5950f..453c068bbeb0 100644
--- a/clang/include/clang/AST/OpenMPClause.h
+++ b/clang/include/clang/AST/OpenMPClause.h
@@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
llvm::SmallVector Sets;
bool anyScoreOrCondition(
- llvm::function_ref Cond) {
-return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet &Set) {
+ const llvm::function_ref &Cond) {
+return llvm::any_of(Sets, [&Cond](OMPTraitInfo::OMPTraitSet &Set) {
return llvm::any_of(
- Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector &Selector) {
+ Set.Selectors, [&Cond](OMPTraitInfo::OMPTraitSelector &Selector) {
return Cond(Selector.ScoreOrCondition,
/* IsScore */ Selector.Kind !=
llvm::omp::TraitSelector::user_condition);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
