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 (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, 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 <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
