[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 17:06:54 PDT 2020 

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, 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
>>>>  >
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200326/7235c96b/attachment-0001.html>

More information about the cfe-commits mailing list