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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Some notes about the DIArgList stuff, otherwise LGTM.



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2100
+      if (I != ValueMapping.end()) {
+        DbgInstruction->replaceVariableLocationOp(DbgOperand, I->second);
+      }
----------------
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).


================
Comment at: llvm/test/Transforms/JumpThreading/thread-debug-info.ll:29
+  %tobool1 = icmp eq i32 %cond2, 0, !dbg !20
+  call void @llvm.dbg.value(metadata !DIArgList(ptr %ptr, i1 %tobool1), metadata !13, metadata !DIExpression()), !dbg !20
+  br i1 %tobool1, label %bb.file, label %bb.f2, !dbg !21
----------------
As long as the test passes this should be fine, but worth noting some functions will assert that the `DIExpression` for a debug value with a `DIArgList` contains operands that reference those arguments; a simple example that would work here would be `!DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140006