[clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused
Arthur O'Dwyer via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 26 15:12:28 PDT 2020
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 <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/c58daf41/attachment-0001.html>
More information about the cfe-commits
mailing list