Fix DbgValue handling in SelectionDAG.

[niravd]

Summary

[DAG] Transfer Debug Values in ReplaceAllUsesWith(SDValue, SDValue) before
modifying CSE maps and check debug values for invalidation
before transfering them.

This fixes PR28613.

[hans]

I don't know this code well enough to review it, but I do know that I want it for 3.9 if it's correct :-)

[niravd]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6659 ↗

(On Diff #65720)

It's not 100% clear to me if these values should invalidated (i.e. moving the values or a copying). We only call TransferDbgValues from ReplaceUses* functions but it is possible that a value who's DbgValues are transferred can theoretically get a use later and so we might want to keep them. That said, intuitively such cases would be from a separate code location and we shouldn't leave the original DebugInfo around.

[echristo]

Can you elaborate on what's going on here a bit more and why this is the correct fix? We've poked at this a few times lately so it's not clear.

Thanks!

-eric

[yaron.keren]

I added a small reproducer for what is probably the same problem in

http://llvm.org/pr28749

[niravd]

Apparently my email didn't get recorded here so I'm reproducing here:

i strongly believe (but have not checked because I had not been able to reproduce the bug locally and have been relying on Ismall running the test) that the sole part of the fix necessary is the change of location TransferDbgValues in ReplaceAllUsesWith to be consistent with the other N instances of ReplaceAllUses and do the transfer before changing the SDUses and doing CSE. This was clearly wrong the dest SDNode may be merged in CSE and the old pointer may point to free space.

Now that I (hopefully) can reproduce this bug with pr28749, I can verify that the second half of the patch is not necessary for correctness and we can potentially split that off.

The second half of the patch regards how we should be Transferring debug Values.

Previous to r273585 when replacing a Node/Value with another we would mostly copy debug values, but not for all replacements which resulted in DebugValues being dropped when doing some optimization as the old node was optimized away and the new node didn't get a cloned version. It appears to compensate for this dropping we would copy all DebugValues associated with the entire SDNode when transferring a SDValue. Since this is debugging information lost during an optimization slightly off debug info isn't obviously wrong.

r273585 changed it so we always clone exactly the DebugValues associated with a SDValue we replaced in the SelectionDAG. The original DebugValues stays associated with the original SDValue. If later transformations merge this node with another it is possible that the original value will remain to the end and we will see multiple instances of the original debug values. I think this is not what we want to do as intuitively all paths associated with the DebugValue would have been transferred and any future uses would come from a different path in the original source. This patch invalidates the original debug values preventing the multiple instances.

[hans]

In D22858#499283, @niravd wrote:

i strongly believe (but have not checked because I had not been able to reproduce the bug locally and have been relying on Ismall running the test) that the sole part of the fix necessary is the change of location TransferDbgValues in ReplaceAllUsesWith to be consistent with the other N instances of ReplaceAllUses and do the transfer before changing the SDUses and doing CSE. This was clearly wrong the dest SDNode may be merged in CSE and the old pointer may point to free space.

Now that I (hopefully) can reproduce this bug with pr28749, I can verify that the second half of the patch is not necessary for correctness and we can potentially split that off.

sgtm. Hopefully splitting it off (if that works) means this will be easy to review and can be merged to 3.9 soon (I'd like to have this before cutting rc1).

[niravd]

My attempts at reproducing the bug from PR28749 have failed so I can't tell if TransferDbgValue is sufficient to fix this. In the interests of resolving this quickly I'm going to commit just that as it's fixing a clear mistake and will leave this patch open to discuss the remainder of this patch.

[hans]

In D22858#499623, @niravd wrote:

My attempts at reproducing the bug from PR28749 have failed so I can't tell if TransferDbgValue is sufficient to fix this.

PR28749 reproduces reliably for me (I tried on the 3.9 branch, but I assume it's the same on trunk).

In the interests of resolving this quickly I'm going to commit just that as it's fixing a clear mistake and will leave this patch open to discuss the remainder of this patch.

IIUC, you committed that in r277027. However, that doesn't fix PR28749 for me. Applying the full patch from this review does make the error go away though, so it seems to be doing the right thing :-)

[yaron.keren]

PR28749 was created with trunk clang on Ubuntu x64.
It reproduced reliably but is very fragile, changing just about anything in the code makes the problem disappear.

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 
 /// TransferDbgValues - Transfer SDDbgValues. Called in replace nodes.
 void SelectionDAG::TransferDbgValues(SDValue From, SDValue To) {
   if (From == To || !From.getNode()->getHasDebugValue())
     return;
   SDNode *FromNode = From.getNode();
   SDNode *ToNode = To.getNode();
   ArrayRef<SDDbgValue *> DVs = GetDbgValues(FromNode);
+  SmallVector<SDDbgValue *, 2> ClonedDVs;
   for (ArrayRef<SDDbgValue *>::iterator I = DVs.begin(), E = DVs.end();
        I != E; ++I) {
     SDDbgValue *Dbg = *I;
     // Only add Dbgvalues attached to same ResNo.
     if (Dbg->getKind() == SDDbgValue::SDNODE &&
-        Dbg->getResNo() == From.getResNo()) {
+        Dbg->getSDNode() == From.getNode() &&
+        Dbg->getResNo() == From.getResNo() && !Dbg->isInvalidated()) {
+      assert(FromNode != ToNode &&
+             "Should not transfer Debug Values intranode");
       SDDbgValue *Clone =
           getDbgValue(Dbg->getVariable(), Dbg->getExpression(), ToNode,
                       To.getResNo(), Dbg->isIndirect(), Dbg->getOffset(),
                       Dbg->getDebugLoc(), Dbg->getOrder());
-      AddDbgValue(Clone, ToNode, false);
+      ClonedDVs.push_back(Clone);
+      Dbg->setIsInvalidated();
     }
   }
+  for (SDDbgValue *I : ClonedDVs)
+    AddDbgValue(I, ToNode, false);
 }
 
 //===----------------------------------------------------------------------===//
 //                              SDNode Class
 //===----------------------------------------------------------------------===//
 
 bool llvm::isNullConstant(SDValue V) {
   ConstantSDNode *Const = dyn_cast<ConstantSDNode>(V);