[clang] 857bf5d - [FIX] Do not copy an llvm::function_ref if it has to be reused

[David Blaikie via cfe-commits]

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.


> The change was not a "shot in the dark" but identified as proper fix by
> examining the `llvm::function_ref` implementation.
>

What aspect of the implementation of llvm::function_ref makes it not safe
to copy in the way the code original did so? My reading of it makes it look
like it's OK to copy so long as it doesn't outlive the functor it is
pointing to (& the code as originall written doesn't look like it's
outliving the caller's functor)


> I could not reproduce the error on any machine I had immediately access
> to so maybe you can call that part "shot in the dark".
>

Sounds like this sort of thing could/should've shown up with msan, maybe?
Were you able to test with any sanitizers?


>
>
> I'm not sure what else you want me to add here as a explanation but
> please let me know if there is anything I can do.
>
>
> On 3/22/20 12:21 AM, David Blaikie wrote:
> > "a problem" seems a smidge vague - was there a specific explanation for
> the
> > kind of problem that was signalled? Or was this fix a bit of a shot in
> the
> > dark?
> >
> > On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <
> > cfe-commits at lists.llvm.org> wrote:
> >
> >> Author: Johannes Doerfert
> >> Date: 2020-02-16T00:51:11-06:00
> >> New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> >>
> >> URL:
> >>
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2
> >> DIFF:
> >>
> https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff
> >>
> >> LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused
> >>
> >> Some buildbots signaled a problem in this method when the
> >> llvm::function_ref was copied and reused after 1228d42ddab8. To
> >> eliminate the problem we avoid copying the llvm::function_ref and
> >> instead we pass it as a const reference.
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >>      clang/include/clang/AST/OpenMPClause.h
> >>
> >> Removed:
> >>
> >>
> >>
> >>
> >>
> ################################################################################
> >> diff  --git a/clang/include/clang/AST/OpenMPClause.h
> >> b/clang/include/clang/AST/OpenMPClause.h
> >> index a3831fd5950f..453c068bbeb0 100644
> >> --- a/clang/include/clang/AST/OpenMPClause.h
> >> +++ b/clang/include/clang/AST/OpenMPClause.h
> >> @@ -6682,10 +6682,10 @@ struct OMPTraitInfo {
> >>     llvm::SmallVector<OMPTraitSet, 4> Sets;
> >>
> >>     bool anyScoreOrCondition(
> >> -      llvm::function_ref<bool(Expr *&, bool /* IsScore */)> Cond) {
> >> -    return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet &Set) {
> >> +      const llvm::function_ref<bool(Expr *&, bool /* IsScore */)>
> &Cond) {
> >> +    return llvm::any_of(Sets, [&Cond](OMPTraitInfo::OMPTraitSet &Set) {
> >>         return llvm::any_of(
> >> -          Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector
> &Selector)
> >> {
> >> +          Set.Selectors, [&Cond](OMPTraitInfo::OMPTraitSelector
> >> &Selector) {
> >>               return Cond(Selector.ScoreOrCondition,
> >>                           /* IsScore */ Selector.Kind !=
> >>                               llvm::omp::TraitSelector::user_condition);
> >>
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200322/e7b460b2/attachment.html>