[clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 26 14:08:14 PDT 2020
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
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200326/35594618/attachment-0001.html>
More information about the cfe-commits
mailing list