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

[David Blaikie]

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 )



>
> 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/623cb2ec/attachment.html>