[PATCH] IR: Move the complex address expression field out of DIVariable and into an extra argument of the dbg.declare/dbg.value intrinsics.

[David Blaikie]

On Tue, Aug 26, 2014 at 5:19 PM, Adrian Prantl <aprantl at apple.com> wrote:

> >>! In D4919#11, @dblaikie wrote:
> >>>! In D4919#10, @aprantl wrote:
> >> If we also bump the metadata version number with this patch, the debug
> info will be stripped. Originally I thought that AsmParser would still
> throw an error because the intrinsics are missing an argument, but this is
> not the case, I just verified that. So this is good, that removes the
> autoupgrade requirement!
> >
> > Excellent!
> >
> >> My rationale for DW_TAG_user_lo is that this way we can guarantee that
> all DIDescriptor tags (except line table entries) are within the range
> [DW_TAG_array_type | LLVM_DebugVersion .. DW_TAG_user_hi|LLVM_DebugVersion].
> >
> > In what way is this beneficial? I suppose it helps us reserve different
> namespaces for different metadata annotations.
>
> Exactly!
>
> > Are we annotating any other metadata? How are we identifying that
> metadata? If we are, I assume we'd want an overall scheme, but I doubt we
> are.
>
> Currently we aren't (in fact, TBAA is the *only* other kind of metadata
> that I'm aware of), but I think it would be nice if we could in the future.
> The -blocks testcases are a nice example of just how useful having a
> human-readable comment can be.
>
> If you feel that using the DW_TAG_user range has too much of a potential
> for conflicts, we could also allocate an adjacent range.
>
> >
> >> My current suggestion is to use the end of the DW_TAG_user range for
> this purpose:
> >> ```
> >>     enum ComplexAddrKind {
> >>       OpFirst = dwarf::DW_TAG_hi_user - dwarf::DW_OP_hi_user,
> >>       OpPlus  = OpFirst + dwarf::DW_OP_plus,
> >>       OpDeref = OpFirst + dwarf::DW_OP_deref,
> >>       OpPiece = OpFirst + dwarf::DW_OP_piece,
> >>       OpLast = OpPiece
> >>     };
> >
> > The reason I shy away from this is that I think, as I mentioned, it
> confuses what these values are - they're not tags, they're just
> identifiers. (I mean I suppose the same is true of
> DW_TAG_arg_variable/param_variable or whatever it is we added that are
> non-standard 'tags' - but that's a bit more justified because they're in
> the same part of the schema - they have to be unique there/in the same
> range)
> >
> >>
> >> ```
>
> ================
> Comment at: include/llvm/CodeGen/MachineModuleInfo.h:394
> @@ -392,3 +393,3 @@
>    /// information of a variable.
> -  void setVariableDbgInfo(MDNode *N, unsigned Slot, DebugLoc Loc) {
> -    VariableDbgInfo Info = { N, Slot, Loc };
> +  void setVariableDbgInfo(MDNode *Expr, MDNode *Var,
> +                          unsigned Slot, DebugLoc Loc) {
> ----------------
> dblaikie wrote:
> > I'd probably put the Var at the start - sort of the broadest thing
> first. But it's not a strong preference & certainly no great argument from
> consistency. (& I won't repeat this feedback for all the various similar
> APIs that have the same ordering choice)
> Whichever way we go, we _have_ to be consistent. The fact that both are
> MDNode pointers is just asking for bugs.
> I went with this order to be consistent with the intrinsic
> llvm.dbg.value(%val, i64 offset, !metadata Var)
> which I extended to
> (1) llvm.dbg.value(%val, i64 offset, !metadata Expr, !metadata Var)
>
> If we go with your suggestion, it would be better to also change the
> intrinsic to
> (2) llvm.dbg.value(%val, i64 offset, !metadata Var, !metadata Expr)
> but that is inconsistent with the position of offset, unless we change it
> to
> (3) llvm.dbg.value(%val, !metadata Var, i64 offset, !metadata Expr)
>
> [That said, one day in the future we may want to roll the offset into
> Expr, so maybe we can skip (3) as an intermediate step.]
>
> My preference is (1) simply because it is consistent with the previous
> intrinsic.
>

OK - thanks for the background. Sounds reasonable to me.


>
> ================
> Comment at: include/llvm/IR/DebugInfo.h:722
> @@ +721,3 @@
> +/// DIExpression - A complex location expression.
> +class DIExpression : public DIDescriptor {
> +  friend class DIDescriptor;
> ----------------
> dblaikie wrote:
> > Does this benefit/need to be a DIDescriptor? I suppose in dumping code
> it might be handy (?) but generally we're never going to have a generic
> DIDescriptor and have to ask if it's a DIExpression. There's only one place
> we'll find them while navigating the debug info metadata - and that one
> place there won't be anything else we expect to find.
> You answered that question yourself :-)
> Yes, it is necessary for dump(), and dump() is essential for the testsuite.
>

Fair enough - at some point, if we're going to generalize the metadata
annotation schema, we might be able to have multiple entry points (obvious
"everything that has metadata annotations is a DIDescriptor" isn't scalable
to non-debug info - so we'd need a more general system anyway).

But perhaps we'll do the "blobification" of debug info (putting most of it
in a single string, etc) before we get there - at which point we'll have a
completely different story for annotating that debug info data anyway.


>
> ================
> Comment at: include/llvm/IR/Metadata.h:34
> @@ -33,3 +33,3 @@
>  enum LLVMConstants : uint32_t {
> -  DEBUG_METADATA_VERSION = 1  // Current debug info version number.
> +  DEBUG_METADATA_VERSION = 2  // Current debug info version number.
>  };
> ----------------
> dblaikie wrote:
> > Did we ever figure out the answer to rolling the version number? I
> assume this is one of the reasons you have so much test fallout - is there
> any way we can commit this change separately? (I realize that'll leave a
> revision or two that don't make sense) just from a review-ability
> perspective? (not that I've looked over the tests - but it'd possibly be
> nicer to be able to at least eyeball a few that actually have schema
> changes versus those that just need the revision number updated)
> IMO we have to bump the version number together with a change that changes
> the metadata layout. Otherwise, if we had
> r1 - base
> r2 - update metadata
> r3 - bump version
>
> users that LTO a module compiled with r1 and a module compiled with r2
> will get an assertion/crash. Now, you might argue that we don't need to
> cater for users living off trunk, but it would also be hard to tell which
> commits warrant a version bump when, e.g, cherry-picking to an llvm-release
> branch. Whatever consensus we reach, please let's document it somewhere :-)
>

This just sounds very churny - I thought the original discussion about
adding this had lead to the idea we might roll this every LLVM release if
the schema had changed. Rolling it for every schema change just seems too
noisy to cope with.

But yes, it should be documented - perhaps we can dredge up the last thread
where we discussed this (I've no doubt forgotten the conclusion)?


>
> ================
> Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:619
> @@ -618,3 +618,3 @@
>
> -  DIVariable V(MI->getOperand(2).getMetadata());
> +  DIVariable V = MI->getDebugVariable();
>    if (V.getContext().isSubprogram()) {
> ----------------
> dblaikie wrote:
> > This looks like cleanup (could be a separate, precursor patch), no?
> Agreed!
>
> ================
> Comment at: lib/IR/DIBuilder.cpp:1087
> @@ +1086,3 @@
> +/// createExpression - Create an empty DIExpression.
> +DIExpression DIBuilder::createExpression() {
> +  return DIExpression(MDNode::get(VMContext, ArrayRef<Value *>()));
> ----------------
> dblaikie wrote:
> > What's this for? (I haven't seen any callers)
>

I'm still a bit curious as to what this is for, though - could you explain
it? I couldn't find any callers - but perhaps I didn't search well enough.


> >
> > If needed, I'd just implement it by having the above function default
> the argument to None:
> >
> > createExpression(Arrayref<Value *> Addr = None)
> Ah, that does look much better, yes! Thanks.
>
> http://reviews.llvm.org/D4919
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140827/f1533b12/attachment.html>