Page MenuHomePhabricator

[DebugInfo] Support multiple location operands via DIArgList in DbgVariableIntrinsics
AcceptedPublic

Authored by StephenTozer on Sep 24 2020, 7:23 AM.

Details

Summary

Following from the previous patch in this series which adds the DIArgList metadata, allowing us to reference multiple Value pointers, this patch updates DbgVariableIntrinsics to make use of this functionality, resulting in a significant change to its interface.

This patch is not intended to update all IR passes to handle dbg.values with multiple location operands; that is being left to the next patch in the stack. This patch simply updates DbgVariableIntrinsic to handle DIArgLists, and updates code elsewhere to use the new interface. In some cases these changes are immediately valid, but in many they are not. All code outside of the intrinsics themselves assumes that an intrinsic will always have exactly one location operand; they will still support DIArgLists, but only if they contain exactly one Value.

Among other changes, the setOperand and setArgOperand functions in DbgVariableIntrinsic have been made private. This is to prevent code from setting the operands of these intrinsics directly, which could easily result in incorrect/invalid operands being set. Obviously this does not prevent these functions from being called on a debug intrinsic at all, as they can still be called on any CallInst pointer. My assumption however is that any code that is looking at a generic pointer without examining the type is going to be conservative about setting the operands to arbitrary values, and in any case where we could safely do so for any CallInst, we can safely do so for a DbgVariableIntrinsic. The intention for making these functions private is to prevent DIArgLists from being overwritten by code that's naively trying to replace one of the Values it points to, and also to fail fast if a DbgVariableIntrinsic is updated to use a DIArgList without a valid corresponding DIExpression.

Diff Detail

Event Timeline

StephenTozer created this revision.Sep 24 2020, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 7:23 AM
aprantl added inline comments.Sep 28 2020, 9:06 AM
llvm/lib/IR/IntrinsicInst.cpp
46

Maybe this isn't such a big deal, but this function has a weird UI now — returning a SmallVector with nullptrs... I wonder if we should instead return a small iterator object? How is this used?

StephenTozer added inline comments.Sep 28 2020, 9:58 AM
llvm/lib/IR/IntrinsicInst.cpp
46

I agree that the UI to this function is weird. Frankly, I don't think this should ever have been returning nullptr in the first place - only a malformed debug intrinsic should have a first argument which isn't a valid MetadataAsValue (this is confirmed by the verifier). I've kept it here only because I was avoiding changing the behaviour of the existing code that uses the nullptr return value. If there must be an "error" response, it also shouldn't be the empty list, because we may want to use that for constants such as:

llvm.dbg.value(!DIArgList(), !"x", !DIExpression(DW_OP_uconst, 5, DW_OP_stack_value))

Putting the nullptr issue aside, it might still be better to return an iterator for this that casts individual arguments to Value* with each iteration. I returned a SmallVector here for simplicity's sake, but for general use it might be easier to define a custom iterator for this class. Do you think it'd be useful in that case?

aprantl accepted this revision.Sep 29 2020, 4:49 PM
aprantl added inline comments.
llvm/lib/IR/IntrinsicInst.cpp
46

I'm sorry for having brought this up — it's always the person who dares touching something who then gets tasked with fixing up our technical debt :-)

Putting the nullptr issue aside, it might still be better to return an iterator for this that casts individual arguments to Value* with each iteration. I returned a SmallVector here for simplicity's sake, but for general use it might be easier to define a custom iterator for this class. Do you think it'd be useful in that case?

The iterator is more elegant, but really the SmallVector affords the exact same UX, which is being able to write for (auto i : fn()). So I think I'll leave this up to you. The iterator will perform slightly better if we have early exists, so that good, but we can also decide to not fix this now and move on.

This revision is now accepted and ready to land.Sep 29 2020, 4:49 PM