<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 26, 2020 at 8:49 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>> wrote:<br></div><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">I'm not sure, but I do see that the call stack contains a call to<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"></span></p><pre style="color:rgb(0,0,0);font-family:"Courier New",courier,monotype,monospace"><span>bool llvm::function_ref<bool (clang::Expr*&, bool)>::callback_fn<llvm::function_ref<bool (clang::Expr*&, bool)> const>(long, clang::Expr*&, bool)<br></span></pre>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.<br>Here's function_ref's copy constructor:<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p><p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> template <typename Callable></span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> function_ref(Callable &&callable,</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> std::enable_if_t<!std::is_same<std::remove_reference_t<Callable>,</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> function_ref>::value> * = nullptr)</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> : callback(callback_fn<typename std::remove_reference<Callable>::type>),</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> callable(reinterpret_cast<intptr_t>(&callable)) {}</span></p><p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p>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.</div><div dir="ltr"><br>Thoughts?<br></div></div></div></blockquote><div><br></div><div>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,</div></div></div></div></div></blockquote><div><br></div><div>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.</div></div></div></blockquote><div><br>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<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)<br><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>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 (<a href="https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/predefined_ops.h#L323" target="_blank">https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/predefined_ops.h#L323</a>) 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.</div><div><br></div><div>With libc++, the predicate is not copied around inside any_of, which is why this was only failing on some buildbots.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div>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)... <br><br>Investigation continues.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">–Arthur<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 26, 2020 at 5:08 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hey Richard - could you help try to diagnose what's happening here?<br><br>I reverted this patch in: <a href="https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb" target="_blank">https://github.com/llvm/llvm-project/commit/0d0b90105f92f6cd9cc7004d565834f4429183fb</a><br>But that did actually cause buildbot failures, such as these ones: <a href="http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491" target="_blank">http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/24491</a><br> eg: <a href="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" target="_blank">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</a><br>I "fixed" these failures blind by reapplying part of this original commit (the lambda capture by reference rather than by value): <a href="https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9" target="_blank">https://github.com/llvm/llvm-project/commit/2ec59a0a40f4ec02e6b2dbe5f12261959c191aa9</a><br><br>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.<br><br>The full buildbot example failure quoted here in case it times out/gets deleted on the buildbot itself:<br><br>******************** TEST 'Clang :: OpenMP/declare_variant_mixed_codegen.cpp' FAILED ********************<br>Script:<br>--<br>: '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<br>: '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<br>: '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<br>--<br>Exit Code: 2<br><br>Command Output (stderr):<br>--<br>Stack dump:<br>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 <br>1. /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/test/OpenMP/declare_variant_mixed_codegen.cpp:38:92: at annotation token<br> #0 0x0000000012e3ebd0 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3ebd0)<br> #1 0x0000000012e3ece8 PrintStackTraceSignalHandler(void*) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3ece8)<br> #2 0x0000000012e3c4e0 llvm::sys::RunSignalHandlers() (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3c4e0)<br> #3 0x0000000012e3d3a4 SignalHandler(int) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x12e3d3a4)<br> #4 0x00007fffb50f04d8 (linux-vdso64.so.1+0x4d8)<br> #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)<br> #6 0x00007fffff5e7fd0 <br> #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)<br> #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)<br> #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)<br>#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)<br>#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)<br>#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)<br>#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)<br>#14 0x00000000148723c4 clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x148723c4)<br>#15 0x000000001487395c clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, bool) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x1487395c)<br>#16 0x0000000014866a20 clang::ParseAST(clang::Sema&, bool, bool) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x14866a20)<br>#17 0x00000000136f6298 clang::ASTFrontendAction::ExecuteAction() (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x136f6298)<br>#18 0x0000000013de8220 clang::CodeGenAction::ExecuteAction() (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x13de8220)<br>#19 0x00000000136fcc64 clang::FrontendAction::Execute() (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x136fcc64)<br>#20 0x00000000136b71c0 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x136b71c0)<br>#21 0x00000000137c9794 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x137c9794)<br>#22 0x00000000109674cc cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x109674cc)<br>#23 0x00000000109635b8 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x109635b8)<br>#24 0x00000000108b838c main (/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/stage1/bin/clang+0x108b838c)<br>#25 0x00007fffb4a55b10 generic_start_main.isra.0 (/lib64/power8/libc.so.6+0x45b10)<br>#26 0x00007fffb4a55d38 __libc_start_main (/lib64/power8/libc.so.6+0x45d38)<br>FileCheck error: '<stdin>' is empty.<br>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<br><br>--<br><br>********************<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 22, 2020 at 7:38 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Reverted in 0d0b90105f92f6cd9cc7004d565834f4429183fb & I'll see what happens with the buildbots.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 22, 2020 at 5:47 PM Johannes Doerfert <<a href="mailto:johannes@jdoerfert.de" target="_blank">johannes@jdoerfert.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Apologies for the confusion.<br>
<br>
I wrote the commit message after looking into this and I though the<br>
issue was related to the capture by copy in the inner llvm::any_of and<br>
the reuse in the outer. Looking back at the code I cannot say anymore<br>
how I got that impression.<br>
<br>
If you think the reference is problematic, I'm totally happy to remove<br>
it. If the windows bots (or any other ones) don't like it we need to<br>
investigate why. As mentioned, I had a problem recreating the problem<br>
locally before.<br>
<br>
Cheers,<br>
Johannes<br>
<br>
<br>
On 3/22/20 1:37 PM, Arthur O'Dwyer wrote:<br>
> On Sun, Mar 22, 2020 at 1:48 PM David Blaikie via cfe-commits <<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
>> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert <br>
<<a href="mailto:johannes@jdoerfert.de" target="_blank">johannes@jdoerfert.de</a>><br>
>> wrote:<br>
>><br>
>>> Some buildbots, I think only Windows buildbots for some reason, crashed<br>
>>> in this function.<br>
>>><br>
>>> The reason, as described, is that an `llvm::function_ref` cannot be<br>
>>> copied and then reused. It just happened to work on almost all<br>
>>> configurations.<br>
>>><br>
>><br>
>> That doesn't seem to be accurate, or if it is there's a lot of <br>
mistakes in<br>
>> the codebase - basically every function_ref parameter I can see in <br>
LLVM is<br>
>> passing by value, not by const ref. The implementation of<br>
>> llvm::function_ref looks quite copyable so long as it doesn't <br>
outlive the<br>
>> functor it is pointing to.<br>
>><br>
><br>
> David is correct. llvm::function_ref, like std::reference_wrapper, is a<br>
> trivially copyable type, and it's designed to be copied.<br>
> Like string_view and reference_wrapper, function_ref is designed to be<br>
> passed by value. Redundantly passing function_ref *again by <br>
reference* is a<br>
> code smell.<br>
><br>
> I've also checked the code here, and it looks like there are only two<br>
> callers of `anyScoreOrCondition` — both in Sema/SemaOpenMP.cpp — and they<br>
> are both fine. FWIW, I would recommend reverting Johannes' change and<br>
> seeing if those Windows buildbots are still unhappy (and if so, why).<br>
><br>
><br>
> Btw, one failure mode I'm aware of, but which I believe is NOT <br>
relevant in<br>
> Johannes' case, is that `function_ref`'s converting constructor behaves<br>
> differently from its copy constructor.<br>
><br>
> int main()<br>
><br>
> {<br>
><br>
> auto lamb = [](){ return 42; };<br>
><br>
> auto sheep = [](){ return 43; };<br>
><br>
> llvm::function_ref<int()> one = lamb;<br>
><br>
><br>
> llvm::function_ref<int()> twoA = one; // twoA refers to lamb<br>
><br>
> llvm::function_ref<short()> twoB = one; // twoB refers to one which<br>
> refers to lamb<br>
><br>
><br>
> one = sheep;<br>
><br>
><br>
> assert(twoA() == 42); // twoA refers to lamb<br>
><br>
> assert(twoB() == 43); // twoB refers to one which refers to sheep<br>
><br>
> }<br>
><br>
> That is, if you have a function that takes a parameter of type<br>
> function_ref<A>, and someone passes it an argument of type <br>
function_ref<B>,<br>
> then inside the function your parameter will be referring to that <br>
argument<br>
> itself instead of to its referent.<br>
> However, in Johannes' particular case, we have no function_refs referring<br>
> to other function_refs. We just make a lambda and take a function_ref to<br>
> it, the end. So I'm pretty sure this pitfall is irrelevant.<br>
><br>
> –Arthur<br>
><br>
<br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div>
</div>
</div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>
</blockquote></div></div>