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? –Arthur On Thu, Mar 26, 2020 at 5:08 PM David Blaikie <dblai...@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 <dblai...@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 <johan...@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@lists.llvm.org> wrote: >>> > >>> >> On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert >>> <johan...@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@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits