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

Adrian Prantl aprantl at apple.com
Tue Aug 26 14:58:34 PDT 2014


> On Aug 26, 2014, at 2:45 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> On Thu, Aug 21, 2014 at 5:54 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!
> 
> But the thing is other metadata might not even start with ints - it
> just happens that our pseudo-DWARF scheme did, so that's what we
> picked up on. For some of these things we could probably be more
> precise based on context and drop the tags (though if we did it'd be
> harder to do the annotated comments, of course).
> 
>>> 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.
> 
> Oh, all the test cases are clear why the annotation comments are
> useful (I tend to test against those rather than the metadata these
> days, because that way the tests are schema-independent (ie: it
> doesn't matter whether we add/remove a field, if it's the 3rd field or
> the 4th field, etc)).
> 
>> 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.
> 
> Looking at this further - I think it might make more sense to just
> pick a special number (probably put it in Dwarf.h:LLVMConstants, and
> make it DW_TAG_arg_variable + 1) as a prefix to the whole expression
> list - rather than having to detect all possible DW_OPs that might
> appear at the start of an expression.

Works for me. It uses more space, but it is more legible and in line with what we did with all the other DIDescriptors.

While I update the patch (and the script to update the testcases), is there anything else that should be addressed/discussed before we can make this change?

-- adrian
> 
>> 
>>> 
>>>> 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)
>>> 
>>>> 
>>>> ```
>> 
>> http://reviews.llvm.org/D4919
>> 
>> 




More information about the llvm-commits mailing list