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

Fri Mar 27 16:54:35 PDT 2020

On Fri, 27 Mar 2020 at 16:35, David Blaikie via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Thu, Mar 26, 2020 at 8: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?
>>>>
>>>
>>> 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<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. 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<T> a; function_ref<T> b = a;" didn't hit the
> converting ctor, it ran the copy ctor as desired. I had to "function_ref<T>
> b = static_cast<const function_ref<T> &&>(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 <dblaikie at gmail.com>
>>>> 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-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 0x0000000012e3ebd0 llvm::sys::PrintStackTrace(llvm::raw_ostream&)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3ebd0)
>>>>>  #1 0x0000000012e3ece8 PrintStackTraceSignalHandler(void*)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3ece8)
>>>>>  #2 0x0000000012e3c4e0 llvm::sys::RunSignalHandlers()
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3c4e0)
>>>>>  #3 0x0000000012e3d3a4 SignalHandler(int)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3d3a4)
>>>>>  #4 0x00007fffb50f04d8 (linux-vdso64.so.1+0x4d8)
>>>>>  #5 0x0000000014df41ac bool llvm::function_ref<bool (clang::Expr*&,
>>>>> bool)>::callback_fn<llvm::function_ref<bool (clang::Expr*&, bool)>
>>>>> const>(long, clang::Expr*&, bool)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14df41ac)
>>>>>  #6 0x00007fffff5e7fd0
>>>>>  #7 0x0000000014e2bfd0 clang::OMPTraitInfo::OMPTraitSelector*
>>>>> std::__find_if<clang::OMPTraitInfo::OMPTraitSelector*,
>>>>> __gnu_cxx::__ops::_Iter_pred<clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&,
>>>>> bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)::operator()(clang::OMPTraitInfo::OMPTraitSet&)
>>>>> const::'lambda'(clang::OMPTraitInfo::OMPTraitSelector&)>
>>>>> >(clang::OMPTraitInfo::OMPTraitSelector*,
>>>>> clang::OMPTraitInfo::OMPTraitSelector*,
>>>>> __gnu_cxx::__ops::_Iter_pred<clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&,
>>>>> bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)::operator()(clang::OMPTraitInfo::OMPTraitSet&)
>>>>> const::'lambda'(clang::OMPTraitInfo::OMPTraitSelector&)>,
>>>>> std::random_access_iterator_tag)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14e2bfd0)
>>>>>  #8 0x0000000014e2c118 bool
>>>>> llvm::any_of<llvm::SmallVector<clang::OMPTraitInfo::OMPTraitSelector, 4u>&,
>>>>> clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&,
>>>>> bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)::operator()(clang::OMPTraitInfo::OMPTraitSet&)
>>>>> const::'lambda'(clang::OMPTraitInfo::OMPTraitSelector&)>(llvm::SmallVector<clang::OMPTraitInfo::OMPTraitSelector,
>>>>> 4u>&, clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&,
>>>>> bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)::operator()(clang::OMPTraitInfo::OMPTraitSet&)
>>>>> const::'lambda'(clang::OMPTraitInfo::OMPTraitSelector&)) (.isra.3726)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14e2c118)
>>>>>  #9 0x0000000014e2c2a4 clang::OMPTraitInfo::OMPTraitSet*
>>>>> std::__find_if<clang::OMPTraitInfo::OMPTraitSet*,
>>>>> __gnu_cxx::__ops::_Iter_pred<clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&, bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)>
>>>>> >(clang::OMPTraitInfo::OMPTraitSet*, clang::OMPTraitInfo::OMPTraitSet*,
>>>>> __gnu_cxx::__ops::_Iter_pred<clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&, bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)>,
>>>>> std::random_access_iterator_tag)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14e2c2a4)
>>>>> #10 0x0000000014e2c368 bool
>>>>> llvm::any_of<llvm::SmallVector<clang::OMPTraitInfo::OMPTraitSet, 4u>&,
>>>>> clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&,
>>>>> bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&)>(llvm::SmallVector<clang::OMPTraitInfo::OMPTraitSet,
>>>>> 4u>&, clang::OMPTraitInfo::anyScoreOrCondition(llvm::function_ref<bool
>>>>> (clang::Expr*&, bool)>)::'lambda'(clang::OMPTraitInfo::OMPTraitSet&))
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14e2c368)
>>>>> #11 0x0000000014e2cc38
>>>>> clang::Sema::checkOpenMPDeclareVariantFunction(clang::OpaquePtr<clang::DeclGroupRef>,
>>>>> clang::Expr*, clang::OMPTraitInfo&, clang::SourceRange)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14e2cc38)
>>>>> #12 0x00000000148fc550
>>>>> clang::Parser::ParseOMPDeclareVariantClauses(clang::OpaquePtr<clang::DeclGroupRef>,
>>>>> llvm::SmallVector<clang::Token, 4u>&, clang::SourceLocation)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x148fc550)
>>>>> #13 0x00000000148fd354
>>>>> clang::Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl(clang::AccessSpecifier&,
>>>>> clang::Parser::ParsedAttributesWithRange&, bool, clang::TypeSpecifierType,
>>>>> clang::Decl*)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x148fd354)
>>>>> #14 0x00000000148723c4
>>>>> clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&,
>>>>> clang::ParsingDeclSpec*)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x148723c4)
>>>>> #15 0x000000001487395c
>>>>> clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&,
>>>>> bool)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x1487395c)
>>>>> #16 0x0000000014866a20 clang::ParseAST(clang::Sema&, bool, bool)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14866a20)
>>>>> #17 0x00000000136f6298 clang::ASTFrontendAction::ExecuteAction()
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x136f6298)
>>>>> #18 0x0000000013de8220 clang::CodeGenAction::ExecuteAction()
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x13de8220)
>>>>> #19 0x00000000136fcc64 clang::FrontendAction::Execute()
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x136fcc64)
>>>>> #20 0x00000000136b71c0
>>>>> clang::CompilerInstance::ExecuteAction(clang::FrontendAction&)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x136b71c0)
>>>>> #21 0x00000000137c9794
>>>>> clang::ExecuteCompilerInvocation(clang::CompilerInstance*)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x137c9794)
>>>>> #22 0x00000000109674cc cc1_main(llvm::ArrayRef<char const*>, char
>>>>> const*, void*)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x109674cc)
>>>>> #23 0x00000000109635b8 ExecuteCC1Tool(llvm::SmallVectorImpl<char
>>>>> const*>&)
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x109635b8)
>>>>> #24 0x00000000108b838c main
>>>>> (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x108b838c)
>>>>> #25 0x00007fffb4a55b10 generic_start_main.isra.0
>>>>> (/lib64/power8/libc.so.6+0x45b10)
>>>>> #26 0x00007fffb4a55d38 __libc_start_main
>>>>> (/lib64/power8/libc.so.6+0x45d38)
>>>>> FileCheck error: '<stdin>' is empty.
>>>>> FileCheck command line:
>>>>>  /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
>>>>>
>>>>> --
>>>>>
>>>>> ********************
>>>>>
>>>>> On Sun, Mar 22, 2020 at 7:38 PM David Blaikie <dblaikie at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what
>>>>>> happens with the buildbots.
>>>>>>
>>>>>> On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert <
>>>>>> johannes at jdoerfert.de> 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 at lists.llvm.org> wrote:
>>>>>>>  >
>>>>>>>  >> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert
>>>>>>> <johannes at jdoerfert.de>
>>>>>>>  >> 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<int()> one = lamb;
>>>>>>>  >
>>>>>>>  >
>>>>>>>  >     llvm::function_ref<int()> twoA = one;    // twoA refers to
>>>>>>> lamb
>>>>>>>  >
>>>>>>>  >     llvm::function_ref<short()> 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<A>, and someone passes it an argument of type
>>>>>>> function_ref<B>,
>>>>>>>  > 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 at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>> _______________________________________________
> 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/20200327/c802a0ca/attachment-0001.html>