<div dir="rtl"><div dir="ltr">Thanks!</div><div dir="ltr">Minor nitpick, ClearSet could be ClearVector, a SmallVector as it's only accessed sequentially.</div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-08-20 21:26 GMT+03:00 Bruno Cardoso Lopes <span dir="ltr"><<a href="mailto:bruno.cardoso@gmail.com" target="_blank">bruno.cardoso@gmail.com</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Yaron,<br>
<br>
Thanks for the catch :-)<br>
Committed a fix in r245590.<br>
<br>
Cheers,<br>
<div class="HOEnZb"><div class="h5"><br>
On Wed, Aug 19, 2015 at 6:08 PM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
> Hi Bruno,<br>
><br>
> I believe there is use-after-free and possibly use of dangling iterators in<br>
> this code.<br>
> Please follow the first iteration of the while loop:<br>
><br>
> while (!worklist.empty()) {<br>
> BasicBlock *ToUpdate = worklist.back();<br>
> worklist.pop_back();<br>
> ...<br>
><br>
> upon entry, ToUpdate == OldSucc which was pushed prior to entering the while<br>
> loop.<br>
> Thus, ClearSet == ValueSet, V is from ClearSet so ValueSet.count(V) must be<br>
> 1, and the erase command will always happen on the first iteration<br>
><br>
> ValueSet.erase(V);<br>
><br>
> since ClearSet == ValueSet this commands will modify ClearSet which was used<br>
> in the ranged for loop.<br>
> That is likely to be incorrect if erase invalidates ClearSet end() iterator<br>
> (which is computed once only at the start of ranged for loop) or the<br>
> SmallPtrSet iterator itself.<br>
><br>
> Even if the loop continues correctly, should the set becomes empty, it's<br>
> completely erased and freed when the DenseMap entry is erased<br>
><br>
> if (ValueSet.empty())<br>
> OverDefinedCache.erase(OI);<br>
><br>
> and the test condition of the ranged for loop I != E will access dead and<br>
> freed memory just before exiting the loop.<br>
><br>
> I do see such use-after-free report from asan on a complicated example case.<br>
><br>
> Yaron<br>
><br>
><br>
> 2015-08-18 19:34 GMT+03:00 Bruno Cardoso Lopes via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>>:<br>
>><br>
>> Author: bruno<br>
>> Date: Tue Aug 18 11:34:27 2015<br>
>> New Revision: 245309<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=245309&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=245309&view=rev</a><br>
>> Log:<br>
>> [LVI] Improve LazyValueInfo compile time performance<br>
>><br>
>> Changes in LoopUnroll in the past six months exposed scalability<br>
>> issues in LazyValueInfo when used from JumpThreading. One internal test<br>
>> that used to take 20s under -O2 now takes 6min.<br>
>><br>
>> This commit change the OverDefinedCache from<br>
>> DenseSet<std::pair<AssertingVH<BasicBlock>, Value*>> to<br>
>> DenseMap<AssertingVH<BasicBlock>, SmallPtrSet<Value *, 4>><br>
>> and reduces compile time down to 1m40s.<br>
>><br>
>> Differential Revision: <a href="http://reviews.llvm.org/D11651" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11651</a><br>
>><br>
>> rdar://problem/21320066<br>
>><br>
>> Modified:<br>
>> llvm/trunk/lib/Analysis/LazyValueInfo.cpp<br>
>><br>
>> Modified: llvm/trunk/lib/Analysis/LazyValueInfo.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LazyValueInfo.cpp?rev=245309&r1=245308&r2=245309&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LazyValueInfo.cpp?rev=245309&r1=245308&r2=245309&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Analysis/LazyValueInfo.cpp (original)<br>
>> +++ llvm/trunk/lib/Analysis/LazyValueInfo.cpp Tue Aug 18 11:34:27 2015<br>
>> @@ -324,8 +324,9 @@ namespace {<br>
>> /// This tracks, on a per-block basis, the set of values that are<br>
>> /// over-defined at the end of that block. This is required<br>
>> /// for cache updating.<br>
>> - typedef std::pair<AssertingVH<BasicBlock>, Value*> OverDefinedPairTy;<br>
>> - DenseSet<OverDefinedPairTy> OverDefinedCache;<br>
>> + typedef DenseMap<AssertingVH<BasicBlock>, SmallPtrSet<Value *, 4>><br>
>> + OverDefinedCacheTy;<br>
>> + OverDefinedCacheTy OverDefinedCache;<br>
>><br>
>> /// Keep track of all blocks that we have ever seen, so we<br>
>> /// don't spend time removing unused blocks from our caches.<br>
>> @@ -359,7 +360,7 @@ namespace {<br>
>> SeenBlocks.insert(BB);<br>
>> lookup(Val)[BB] = Result;<br>
>> if (Result.isOverdefined())<br>
>> - OverDefinedCache.insert(std::make_pair(BB, Val));<br>
>> + OverDefinedCache[BB].insert(Val);<br>
>> }<br>
>><br>
>> LVILatticeVal getBlockValue(Value *Val, BasicBlock *BB);<br>
>> @@ -425,14 +426,16 @@ namespace {<br>
>> } // end anonymous namespace<br>
>><br>
>> void LVIValueHandle::deleted() {<br>
>> - typedef std::pair<AssertingVH<BasicBlock>, Value*> OverDefinedPairTy;<br>
>> -<br>
>> - SmallVector<OverDefinedPairTy, 4> ToErase;<br>
>> - for (const OverDefinedPairTy &P : Parent->OverDefinedCache)<br>
>> - if (P.second == getValPtr())<br>
>> - ToErase.push_back(P);<br>
>> - for (const OverDefinedPairTy &P : ToErase)<br>
>> - Parent->OverDefinedCache.erase(P);<br>
>> + SmallVector<AssertingVH<BasicBlock>, 4> ToErase;<br>
>> + for (auto &I : Parent->OverDefinedCache) {<br>
>> + SmallPtrSetImpl<Value *> &ValueSet = I.second;<br>
>> + if (ValueSet.count(getValPtr()))<br>
>> + ValueSet.erase(getValPtr());<br>
>> + if (ValueSet.empty())<br>
>> + ToErase.push_back(I.first);<br>
>> + }<br>
>> + for (auto &BB : ToErase)<br>
>> + Parent->OverDefinedCache.erase(BB);<br>
>><br>
>> // This erasure deallocates *this, so it MUST happen after we're done<br>
>> // using any and all members of *this.<br>
>> @@ -446,15 +449,11 @@ void LazyValueInfoCache::eraseBlock(Basi<br>
>> return;<br>
>> SeenBlocks.erase(I);<br>
>><br>
>> - SmallVector<OverDefinedPairTy, 4> ToErase;<br>
>> - for (const OverDefinedPairTy& P : OverDefinedCache)<br>
>> - if (P.first == BB)<br>
>> - ToErase.push_back(P);<br>
>> - for (const OverDefinedPairTy &P : ToErase)<br>
>> - OverDefinedCache.erase(P);<br>
>> + auto ODI = OverDefinedCache.find(BB);<br>
>> + if (ODI != OverDefinedCache.end())<br>
>> + OverDefinedCache.erase(ODI);<br>
>><br>
>> - for (std::map<LVIValueHandle, ValueCacheEntryTy>::iterator<br>
>> - I = ValueCache.begin(), E = ValueCache.end(); I != E; ++I)<br>
>> + for (auto I = ValueCache.begin(), E = ValueCache.end(); I != E; ++I)<br>
>> I->second.erase(BB);<br>
>> }<br>
>><br>
>> @@ -483,8 +482,7 @@ bool LazyValueInfoCache::hasBlockValue(V<br>
>> return true;<br>
>><br>
>> LVIValueHandle ValHandle(Val, this);<br>
>> - std::map<LVIValueHandle, ValueCacheEntryTy>::iterator I =<br>
>> - ValueCache.find(ValHandle);<br>
>> + auto I = ValueCache.find(ValHandle);<br>
>> if (I == ValueCache.end()) return false;<br>
>> return I->second.count(BB);<br>
>> }<br>
>> @@ -1053,10 +1051,10 @@ void LazyValueInfoCache::threadEdge(Basi<br>
>> std::vector<BasicBlock*> worklist;<br>
>> worklist.push_back(OldSucc);<br>
>><br>
>> - DenseSet<Value*> ClearSet;<br>
>> - for (OverDefinedPairTy &P : OverDefinedCache)<br>
>> - if (P.first == OldSucc)<br>
>> - ClearSet.insert(P.second);<br>
>> + auto I = OverDefinedCache.find(OldSucc);<br>
>> + if (I == OverDefinedCache.end())<br>
>> + return; // Nothing to process here.<br>
>> + SmallPtrSetImpl<Value *> &ClearSet = I->second;<br>
>><br>
>> // Use a worklist to perform a depth-first search of OldSucc's<br>
>> successors.<br>
>> // NOTE: We do not need a visited list since any blocks we have already<br>
>> @@ -1072,9 +1070,12 @@ void LazyValueInfoCache::threadEdge(Basi<br>
>> bool changed = false;<br>
>> for (Value *V : ClearSet) {<br>
>> // If a value was marked overdefined in OldSucc, and is here too...<br>
>> - DenseSet<OverDefinedPairTy>::iterator OI =<br>
>> - OverDefinedCache.find(std::make_pair(ToUpdate, V));<br>
>> - if (OI == OverDefinedCache.end()) continue;<br>
>> + auto OI = OverDefinedCache.find(ToUpdate);<br>
>> + if (OI == OverDefinedCache.end())<br>
>> + continue;<br>
>> + SmallPtrSetImpl<Value *> &ValueSet = OI->second;<br>
>> + if (!ValueSet.count(V))<br>
>> + continue;<br>
>><br>
>> // Remove it from the caches.<br>
>> ValueCacheEntryTy &Entry = ValueCache[LVIValueHandle(V, this)];<br>
>> @@ -1082,7 +1083,9 @@ void LazyValueInfoCache::threadEdge(Basi<br>
>><br>
>> assert(CI != Entry.end() && "Couldn't find entry to update?");<br>
>> Entry.erase(CI);<br>
>> - OverDefinedCache.erase(OI);<br>
>> + ValueSet.erase(V);<br>
>> + if (ValueSet.empty())<br>
>> + OverDefinedCache.erase(OI);<br>
>><br>
>> // If we removed anything, then we potentially need to update<br>
>> // blocks successors too.<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Bruno Cardoso Lopes<br>
<a href="http://www.brunocardoso.cc" rel="noreferrer" target="_blank">http://www.brunocardoso.cc</a><br>
</font></span></blockquote></div><br></div>