[llvm] r221709 - Totally forget deallocated SDNodes in SDDbgInfo.

[David Blaikie]

On Tue, Nov 11, 2014 at 2:16 PM, Frédéric Riss <friss at apple.com> wrote:

>
> On Nov 11, 2014, at 1:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Nov 11, 2014 at 1:21 PM, Frederic Riss <friss at apple.com> wrote:
>
>> Author: friss
>> Date: Tue Nov 11 15:21:08 2014
>> New Revision: 221709
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=221709&view=rev
>> Log:
>> Totally forget deallocated SDNodes in SDDbgInfo.
>>
>> What would happen before that commit is that the SDDbgValues associated
>> with
>> a deallocated SDNode would be marked Invalidated, but SDDbgInfo would keep
>> a map entry keyed by the SDNode pointer pointing to this list of
>> invalidated
>> SDDbgNodes. As the memory gets reused, the list might get wrongly
>> associated
>> with another new SDNode. As the SDDbgValues are cloned when they are
>> transfered,
>> this can lead to an exponential number of SDDbgValues being produced
>> during
>> DAGCombine like in http://llvm.org/bugs/show_bug.cgi?id=20893
>>
>> Note that the previous behavior wasn't really buggy as the invalidation
>> made
>> sure that the SDDbgValues won't be used. This commit can be considered a
>> memory optimization and as such is really hard to validate in a unit-test.
>>
>
> This was sort-of undefined behavior previously (comparing a previously
> free'd pointer to a new allocation) but is still sort-of detectable. If we
> added an assertion into some point of initial construction or SDNode usage,
> that there were zero valuse in its map - then this assertion would fire on
> the buggy code? (but only when the allocation got lucky and put it in the
> same place) But would at least fail rather than behave strangely (leakily,
> mind you, but strangely)
>
> I imagine at the call to setHasDebugValue, if it previously had
> !getHasDebugValue, then the number of entries in the map should be 1 (this
> should be the first thing we've added to the map), something like changing
> this:
>
>   DbgInfo->add(DB, SD, isParameter);  if (SD)    SD->setHasDebugValue(true);
>
>
> to this:
>
>
>   if (SD) {    assert(DbgInfo->getSDDbgValues(SD).empty() || !SD->getHasDebugValue());
>     SD->setHasDebugValue(true);
>   }
>   DbgInfo->add(DB, SD, isParameter);
> I agree that adding a test case may still be questionable, given the need for this to end up getting a matching allocation, but maybe there are some simple cases that pretty obviously get the same memory address because they're back-to-back release-then-reallocate? (I think I added a test case like this when I fixed some similar bugs in ArgPromo or DAE, maybe... one of them has a two-pass algorithm that had some invalidity going on - http://llvm.org/viewvc/llvm-project?rev=219210&view=rev )
>
>
> Sorry, I missed that part of the mail. With the assert in place, it might
> be possible to come up with an IR that reproduces this behavior, but as it
> relies on allocation order it would be quite fragile I suppose (Note that I
> do know absolutely nothing about SDAG allocation strategy). I’ll try to toy
> with it when I find some time, but no promises…
>

Yep - see my example from ArgPromo - in that case an easy example existed
due to the deallocation and immediate reallocation (of course this test
case won't always exercise the problematic case - different memory
allocators could easily defeat it - but it seems at least simple/plausible
enough to include (adding appropriate comments & caveats to the test case
in case it entirely rots, so someone knows they can remove it in that case))

I'd expect the case where the allocation is reused is a fairly simple one
like the ArgPromo issue I hit, but if it doesn't fall out so easily,
certainly don't spend a lot of time on it. Though I wouldn't mind if you
looked at it sooner rather than later - at least enough to decide one way
or the other. Unspecified times in the future are easily forgotten (but you
know, doesn't have to be today - but within a week or so seems reasonable,
beyond that and I imagine it'll get lost).

- David


>
> Fred
>
>
>> Modified:
>>     llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
>>     llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>
>> Modified: llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SelectionDAG.h?rev=221709&r1=221708&r2=221709&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/CodeGen/SelectionDAG.h (original)
>> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Tue Nov 11 15:21:08
>> 2014
>> @@ -128,6 +128,10 @@ public:
>>        DbgValMap[Node].push_back(V);
>>    }
>>
>> +  /// \brief Invalidate all DbgValues attached to the node and remove
>> +  /// it from the Node-to-DbgValues map.
>> +  void erase(const SDNode *Node);
>> +
>>    void clear() {
>>      DbgValMap.clear();
>>      DbgValues.clear();
>>
>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp?rev=221709&r1=221708&r2=221709&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Tue Nov 11
>> 15:21:08 2014
>> @@ -687,6 +687,15 @@ void SelectionDAG::DeleteNodeNotInCSEMap
>>    DeallocateNode(N);
>>  }
>>
>> +void SDDbgInfo::erase(const SDNode *Node) {
>> +  DbgValMapType::iterator I = DbgValMap.find(Node);
>> +  if (I == DbgValMap.end())
>> +    return;
>> +  for (auto &Val: I->second)
>> +    Val->setIsInvalidated();
>> +  DbgValMap.erase(I);
>> +}
>> +
>>  void SelectionDAG::DeallocateNode(SDNode *N) {
>>    if (N->OperandsNeedDelete)
>>      delete[] N->OperandList;
>> @@ -697,10 +706,9 @@ void SelectionDAG::DeallocateNode(SDNode
>>
>>    NodeAllocator.Deallocate(AllNodes.remove(N));
>>
>> -  // If any of the SDDbgValue nodes refer to this SDNode, invalidate
>> them.
>> -  ArrayRef<SDDbgValue*> DbgVals = DbgInfo->getSDDbgValues(N);
>> -  for (unsigned i = 0, e = DbgVals.size(); i != e; ++i)
>> -    DbgVals[i]->setIsInvalidated();
>> +  // If any of the SDDbgValue nodes refer to this SDNode, invalidate
>> +  // them and forget about that node.
>> +  DbgInfo->erase(N);
>>  }
>>
>>  #ifndef NDEBUG
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141111/bf23db9c/attachment.html>