<div dir="ltr"><div dir="ltr">On Sun, Mar 22, 2020 at 10:40 AM Johannes Doerfert <<a href="mailto:johannes@jdoerfert.de">johannes@jdoerfert.de</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Some buildbots, I think only Windows buildbots for some reason, crashed <br>
in this function.<br>
<br>
The reason, as described, is that an `llvm::function_ref` cannot be <br>
copied and then reused. It just happened to work on almost all <br>
configurations.<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The change was not a "shot in the dark" but identified as proper fix by <br>
examining the `llvm::function_ref` implementation.<br></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I could not reproduce the error on any machine I had immediately access <br>
to so maybe you can call that part "shot in the dark".<br></blockquote><div><br>Sounds like this sort of thing could/should've shown up with msan, maybe? Were you able to test with any sanitizers?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
I'm not sure what else you want me to add here as a explanation but <br>
please let me know if there is anything I can do.<br>
<br>
<br>
On 3/22/20 12:21 AM, David Blaikie wrote:<br>
> "a problem" seems a smidge vague - was there a specific explanation for the<br>
> kind of problem that was signalled? Or was this fix a bit of a shot in the<br>
> dark?<br>
><br>
> On Sat, Feb 15, 2020 at 10:55 PM Johannes Doerfert via cfe-commits <<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
>> Author: Johannes Doerfert<br>
>> Date: 2020-02-16T00:51:11-06:00<br>
>> New Revision: 857bf5da35af8e1f9425e1865dab5f5fce5e38f2<br>
>><br>
>> URL:<br>
>> <a href="https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2</a><br>
>> DIFF:<br>
>> <a href="https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/857bf5da35af8e1f9425e1865dab5f5fce5e38f2.diff</a><br>
>><br>
>> LOG: [FIX] Do not copy an llvm::function_ref if it has to be reused<br>
>><br>
>> Some buildbots signaled a problem in this method when the<br>
>> llvm::function_ref was copied and reused after 1228d42ddab8. To<br>
>> eliminate the problem we avoid copying the llvm::function_ref and<br>
>> instead we pass it as a const reference.<br>
>><br>
>> Added:<br>
>><br>
>><br>
>> Modified:<br>
>>      clang/include/clang/AST/OpenMPClause.h<br>
>><br>
>> Removed:<br>
>><br>
>><br>
>><br>
>><br>
>> ################################################################################<br>
>> diff  --git a/clang/include/clang/AST/OpenMPClause.h<br>
>> b/clang/include/clang/AST/OpenMPClause.h<br>
>> index a3831fd5950f..453c068bbeb0 100644<br>
>> --- a/clang/include/clang/AST/OpenMPClause.h<br>
>> +++ b/clang/include/clang/AST/OpenMPClause.h<br>
>> @@ -6682,10 +6682,10 @@ struct OMPTraitInfo {<br>
>>     llvm::SmallVector<OMPTraitSet, 4> Sets;<br>
>><br>
>>     bool anyScoreOrCondition(<br>
>> -      llvm::function_ref<bool(Expr *&, bool /* IsScore */)> Cond) {<br>
>> -    return llvm::any_of(Sets, [Cond](OMPTraitInfo::OMPTraitSet &Set) {<br>
>> +      const llvm::function_ref<bool(Expr *&, bool /* IsScore */)> &Cond) {<br>
>> +    return llvm::any_of(Sets, [&Cond](OMPTraitInfo::OMPTraitSet &Set) {<br>
>>         return llvm::any_of(<br>
>> -          Set.Selectors, [Cond](OMPTraitInfo::OMPTraitSelector &Selector)<br>
>> {<br>
>> +          Set.Selectors, [&Cond](OMPTraitInfo::OMPTraitSelector<br>
>> &Selector) {<br>
>>               return Cond(Selector.ScoreOrCondition,<br>
>>                           /* IsScore */ Selector.Kind !=<br>
>>                               llvm::omp::TraitSelector::user_condition);<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
>><br>
</blockquote></div></div>