[PATCH] D140006: fix jump threading failing to update cloned dbg.values

[Ben Mudd via Phabricator via llvm-commits]

BenJMudd added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2100
+      if (I != ValueMapping.end()) {
+        DbgInstruction->replaceVariableLocationOp(DbgOperand, I->second);
+      }
----------------
StephenTozer wrote:
> Unfortunately the use of `replaceVariableLocationOp` here might invalidate the above iterator - specifically after calling this, `DbgInstruction` will have a new `DIArgList` operand but the iterator in this loop will be pointing to the old `DIArgList`. This will cause an error if an instruction to be replaced appears multiple times in the debug value: If you're replacing `%0` with `%1` in `DIArgList(%0, %0)`, then in the first iteration the arglist will be updated to `!DIArgList(%1, %1)`, then in the second iteration it will try to replace `%0` again, which would trigger an assertion.
> 
> A fix for this would be to either build a list of values to be replaced inside the loop and then updating `DbgInstruction` outside of the loop, or iterating over indices in the loop and using the index version of `replaceVariableLocationOp` (personally I prefer the former, but either works).
Ah I hadn't thought of the !DIArgList referencing the same value, it is definitely much safer to change the list outside of the loop. I also prefer the former, thanks 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140006/new/

https://reviews.llvm.org/D140006