<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 17, 2015 at 12:58 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Do you think this fix and test case should be merged to the 3.7 branch?<br></blockquote><div><br></div><div>I think yes. This is a pretty easy fix, and fixes a bug I found in the wild. That bug will trigger for any codebase containing a cmpxchg loop when compiled with `-flto -fno-inline`, unless they directly use the __builtin_ for the atomic operation in the loop (i.e. they don't have a wrapper function around it).</div><div><br></div><div>Obviously `-flto -fno-inline` is not a terribly wise configuration in the first place, but we should at least not miscompile.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
On Wed, Jul 15, 2015 at 5:24 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
> Thanks!<br>
><br>
</span><span class="">> On Wed, Jul 15, 2015 at 5:23 PM Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
>><br>
>> Added in r242356.<br>
>><br>
>> -- Sean Silva<br>
>><br>
</span>>> On Wed, Jul 15, 2015 at 3:49 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>> On Wed, Jul 15, 2015 at 2:18 PM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>><br>
<span class="">>>> wrote:<br>
>>>><br>
>>>> Awesome! Do you want to land that test case? If not, I will, but you may<br>
>>>> get to it sooner at this point...<br>
>>><br>
>>><br>
>>> Sure, I'll do that now.<br>
>>><br>
>>> -- Sean Silva<br>
>>><br>
>>>><br>
>>>><br>
</span>>>>> On Wed, Jul 15, 2015 at 2:08 PM Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>><br>
<span class="">>>>> wrote:<br>
>>>>><br>
>>>>> Thanks! I think this fixed PR23345; I didn't know about<br>
>>>>> I.mayWriteToMemory()<br>
>>>>><br>
>>>>> -- Sean Silva<br>
>>>>><br>
</span>>>>>> On Wed, Jul 15, 2015 at 1:53 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>><br>
<span class="">>>>>> wrote:<br>
>>>>>><br>
>>>>>> Author: chandlerc<br>
>>>>>> Date: Wed Jul 15 03:53:29 2015<br>
>>>>>> New Revision: 242281<br>
>>>>>><br>
</span>>>>>>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D242281-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=8sitU5l_O1sK79Z3UAXUoREha1fM1Abekbz4IgaAYgM&s=wj4e4dzqO7wyc59dAhdRWNgq0AqLNgxovVEfeu_tWZY&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=242281&view=rev</a><br>
<div><div class="h5">>>>>>><br>
>>>>>><br>
>>>>>> Log:<br>
>>>>>> [PM/AA] Fix *numerous* serious bugs in GlobalsModRef found by<br>
>>>>>> inspection.<br>
>>>>>><br>
>>>>>> While we want to handle calls specially in this code because they<br>
>>>>>> should<br>
>>>>>> have been modeled by the call graph analysis that precedes it, we<br>
>>>>>> should<br>
>>>>>> *not* be re-implementing the predicates for whether an instruction<br>
>>>>>> reads<br>
>>>>>> or writes memory. Those are well defined already. Notably, at least<br>
>>>>>> the<br>
>>>>>> following issues seem to be clearly missed before:<br>
>>>>>> - Ordered atomic loads can "write" to memory by causing writes from<br>
>>>>>> other<br>
>>>>>> threads to become visible. Similarly for ordered atomic stores.<br>
>>>>>> - AtomicRMW instructions quite obviously both read and write to<br>
>>>>>> memory.<br>
>>>>>> - AtomicCmpXchg instructions also read and write to memory.<br>
>>>>>> - Fences read and write to memory.<br>
>>>>>> - Invokes of intrinsics or memory allocation functions.<br>
>>>>>><br>
>>>>>> I don't have any test cases, and I suspect this has never really come<br>
>>>>>> up<br>
>>>>>> in the real world. But there is no reason why it wouldn't, and it<br>
>>>>>> makes<br>
>>>>>> the code simpler to do this the right way.<br>
>>>>>><br>
>>>>>> While here, I've tried to make the loops significantly simpler as well<br>
>>>>>> and added helpful comments as to what is going on.<br>
>>>>>><br>
>>>>>> Modified:<br>
>>>>>> llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp<br>
>>>>>><br>
>>>>>> Modified: llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp<br>
>>>>>><br>
>>>>>> URL:<br>
</div></div><div><div class="h5">>>>>>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Analysis_IPA_GlobalsModRef.cpp-3Frev-3D242281-26r1-3D242280-26r2-3D242281-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=8sitU5l_O1sK79Z3UAXUoREha1fM1Abekbz4IgaAYgM&s=P3nmpBs7bhFsbgk_Loalz59V4HHKLU33aJQ6-1Cw8U0&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp?rev=242281&r1=242280&r2=242281&view=diff</a><br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> ==============================================================================<br>
>>>>>> --- llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp (original)<br>
>>>>>> +++ llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp Wed Jul 15 03:53:29<br>
>>>>>> 2015<br>
>>>>>> @@ -439,30 +439,39 @@ void GlobalsModRef::AnalyzeCallGraph(Cal<br>
>>>>>> }<br>
>>>>>><br>
>>>>>> // Scan the function bodies for explicit loads or stores.<br>
>>>>>> - for (unsigned i = 0, e = SCC.size(); i != e && FunctionEffect !=<br>
>>>>>> ModRef;<br>
>>>>>> - ++i)<br>
>>>>>> - for (inst_iterator II = inst_begin(SCC[i]->getFunction()),<br>
>>>>>> - E = inst_end(SCC[i]->getFunction());<br>
>>>>>> - II != E && FunctionEffect != ModRef; ++II)<br>
>>>>>> - if (LoadInst *LI = dyn_cast<LoadInst>(&*II)) {<br>
>>>>>> + for (auto *Node : SCC) {<br>
>>>>>> + if (FunctionEffect == ModRef)<br>
>>>>>> + break; // The mod/ref lattice saturates here.<br>
>>>>>> + for (Instruction &I : inst_range(Node->getFunction())) {<br>
>>>>>> + if (FunctionEffect == ModRef)<br>
>>>>>> + break; // The mod/ref lattice saturates here.<br>
>>>>>> +<br>
>>>>>> + // We handle calls specially because the graph-relevant<br>
>>>>>> aspects are<br>
>>>>>> + // handled above.<br>
>>>>>> + if (auto CS = CallSite(&I)) {<br>
>>>>>> + if (isAllocationFn(&I, TLI) || isFreeCall(&I, TLI)) {<br>
>>>>>> + // FIXME: It is completely unclear why this is necessary<br>
>>>>>> and not<br>
>>>>>> + // handled by the above graph code.<br>
>>>>>> + FunctionEffect |= ModRef;<br>
>>>>>> + } else if (Function *Callee = CS.getCalledFunction()) {<br>
>>>>>> + // The callgraph doesn't include intrinsic calls.<br>
>>>>>> + if (Callee->isIntrinsic()) {<br>
>>>>>> + ModRefBehavior Behaviour =<br>
>>>>>> + AliasAnalysis::getModRefBehavior(Callee);<br>
>>>>>> + FunctionEffect |= (Behaviour & ModRef);<br>
>>>>>> + }<br>
>>>>>> + }<br>
>>>>>> + continue;<br>
>>>>>> + }<br>
>>>>>> +<br>
>>>>>> + // All non-call instructions we use the primary predicates<br>
>>>>>> for whether<br>
>>>>>> + // thay read or write memory.<br>
>>>>>> + if (I.mayReadFromMemory())<br>
>>>>>> FunctionEffect |= Ref;<br>
>>>>>> - if (LI->isVolatile())<br>
>>>>>> - // Volatile loads may have side-effects, so mark them as<br>
>>>>>> writing<br>
>>>>>> - // memory (for example, a flag inside the processor).<br>
>>>>>> - FunctionEffect |= Mod;<br>
>>>>>> - } else if (StoreInst *SI = dyn_cast<StoreInst>(&*II)) {<br>
>>>>>> + if (I.mayWriteToMemory())<br>
>>>>>> FunctionEffect |= Mod;<br>
>>>>>> - if (SI->isVolatile())<br>
>>>>>> - // Treat volatile stores as reading memory somewhere.<br>
>>>>>> - FunctionEffect |= Ref;<br>
>>>>>> - } else if (isAllocationFn(&*II, TLI) || isFreeCall(&*II,<br>
>>>>>> TLI)) {<br>
>>>>>> - FunctionEffect |= ModRef;<br>
>>>>>> - } else if (IntrinsicInst *Intrinsic =<br>
>>>>>> dyn_cast<IntrinsicInst>(&*II)) {<br>
>>>>>> - // The callgraph doesn't include intrinsic calls.<br>
>>>>>> - Function *Callee = Intrinsic->getCalledFunction();<br>
>>>>>> - ModRefBehavior Behaviour =<br>
>>>>>> AliasAnalysis::getModRefBehavior(Callee);<br>
>>>>>> - FunctionEffect |= (Behaviour & ModRef);<br>
>>>>>> - }<br>
>>>>>> + }<br>
>>>>>> + }<br>
>>>>>><br>
>>>>>> if ((FunctionEffect & Mod) == 0)<br>
>>>>>> ++NumReadMemFunctions;<br>
>>>>>><br>
>>>>>><br>
>>>>>> _______________________________________________<br>
>>>>>> llvm-commits mailing list<br>
</div></div>>>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<span class="">>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>>><br>
>>>>> _______________________________________________<br>
>>>>> llvm-commits mailing list<br>
</span>>>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<span class="">>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>><br>
>>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
</span>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<span class="">>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
</span>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</blockquote></div><br></div></div>