At the moment, debug uses of an Instruction are replaced with Undef in Instruction’s destructor (Instruction.cpp). My question is: is there a reason that we don’t do this in Value’s destructor instead, since Instruction’s dtor doesn’t cover cases such as use of a dead global?
(Alternative suggestion at end of post)
Here a contrived example that shows how we end up with misleading debug info (alternatively view in Compiler Explorer):
static void empty() {}
void ext();
void fun() {
auto f = &ext; // dbg.value(ext ptr, ...)
f();
f = ∅ // dbg.value(empty ptr, ...)
f();
ext();
}
Compiled with -O2 -g we end up with:
define dso_local void @_Z3funv() local_unnamed_addr #0 !dbg !9 {
call void @llvm.dbg.value(metadata ptr @_Z3extv, metadata !14, metadata !DIExpression()), !dbg !16
tail call void @_Z3extv(), !dbg !17
; NOTE: Ideally there should be a dbg.value(undef) here.
tail call void @_Z3extv(), !dbg !18
ret void, !dbg !19
}
The llvm.dbg.value intrinsic for the second assignment to f is dropped due to this sequence of events:
SROA promotes the function pointer store-loads with the function pointer value (the indirect calls become direct calls)
The call to empty is removed by the inliner.
empty has internal linkage and no users so is deleted.
The llvm.dbg.value describing the second assignment has its value (ptr to empty) replaced with empty metadata.
The empty metadata operand signals that the debug intrinsic is fair game to be cleaned up, so it is deleted by EarlyCSEPass .
An alternative strategy could be to redefine the semantics of empty metadata in debug intrinsics. We could say that empty metadata in place of a value means what Undef currently means (the value / location of the variable is unknown). Then we don’t need to perform this RAUW at all. It’s just a thought though and I’ve not done any experimenting for that - opinions on this welcome too.
And here is a simpler example that demonstrates a dropped intrinsic but without the additional scaffolding required to produce incorrect debug info (since an undef dbg.value here is essentially equivalent to no dbg.value):
static void empty() {}
void fun() { auto e = ∅ }
Thanks for highlighting this case! After looking through your example, I agree it seems best to move the auto-undef to Value to cover cases such as this.
Hmm, I am still relatively new to the debug info portion of LLVM, so it’s hard for me to imagine the full impact of this potential change in semantics… Are there any cases where we do want to allow empty debug value inputs to still be cleaned up? If yes, then it suggests to me that we should leave the semantics as there are, since it shows that the different meanings of empty vs. undef are useful.
Based on recent discussions I think we’re probably going to need to change the auto-undef mechanism to auto-poison instead. So, if we do choose to go down that route (rather than change empty-metadata semantics) that change should happen first, i.e. change Instruction’s dtor to auto-poison, fixup tests etc, then move the behaviour from Instruction to Value’s dtor.
I get the feeling that the behaviour is just a historical artefact of the codebase, but I can’t say that with certainty. Going down the redefining-empty-metadata-semantics route would be more disruptive, so maintaining the auto-undef/poison route for now makes sense until someone has time to have a deeper look IMO (even though I do slightly prefer that idea from a design standpoint at the moment).
I’ve had some time to think about and look into this more closely. I considered disallowing (verifying against) empty metadata operands and moving the auto-undef behaviour into ~Value (as mentioned in previous comments). This has the downside of working against the grain of LLVM’s infrastructure, which tends to null-out Values in a variety of circumstances which we’d need to fix on a case by case basis.
I liked the idea of using empty metadata !{} as the One True Kill Sentinel. However, there are a number of constraints I had not considered in my initial post.
undefs may occur naturally
Undef (and/or poison) is not only an intentional “kill sentinel” in debug intrinsics that marks a terminating location, but may also occur naturally when a value truly is undef (and/or poison). In my opinion we should continue to treat “naturally” undef’d debug intrinsics as kill sentinels. With that in mind, we can not eliminate undef as a “kill sentinel”.
DIArgLists require their own representation
First, replacing a DIArgList use with an empty metadata node looks like it can only be achieved in a hacky, ugly way. So right off the bat we would need a separate kill sentinel to non-variadic debug intrinsics if they use !{}.
Second, DIArgLists are set up to wrap Values rather than metadata, which means we need to do extreme hoop jumping (or re-writing) if we want to be able to represent !{} within the lists.
With these facts in mind, I believe the best course of action is:
Treat !{} as kill sentinels (so do not delete the intrinsics with !{} operands!)
Continue treating undef as a kill sentinel.
Keep DIArgLits::handleChangedOperand’s conversion of nullptr → undef
Optionally: delete ~Instruction’s auto-undefing behaviour now that !{} is a kill sentinel.
While I’m in the area, we can also replace lots sites introducing undef to use poison instead (cc @nlopes).
I’ve written up some patches that achieve this, which I’ll post shortly.
The patch stack starts at ⚙ D140900 [DebugInfo] Print empty MDTuples wrapped in MetadataAsValue inline. There is at least one more usage of UndefValue that can be replaced with PoisonValue that I know of - DIArgList::handleChangedOperand - which I’ll add a patch for tomorrow (and there probably a bunch more I don’t know about off the top of my head).