[PATCH] AsmWriter/Bitcode: Support specialized debug nodes

Thu Feb 12 18:02:23 PST 2015

> On 2015-Feb-12, at 16:18, Adrian Prantl <aprantl at apple.com> wrote:
> 
>> 
>> On Feb 12, 2015, at 4:07 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> 
>>> On 2015-Feb-11, at 08:25, Adrian Prantl <aprantl at apple.com> wrote:
>>> 
>>>> 
>>>> On Feb 10, 2015, at 6:29 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On 2015-Feb-10, at 16:59, Adrian Prantl <aprantl at apple.com> wrote:
>>>>> 
>>>>> 
>>>>>> On Feb 10, 2015, at 4:48 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 2015-Feb-10, at 16:05, Adrian Prantl <aprantl at apple.com> wrote:
>>>>>>> 
>>>>>>> A few nitpicky suggestions and questions:
>>>>>>> 
>>>>>>> - !MDSubrange(count: 30, lo: 2)
>>>>>>> what about spelling out “low” and “high”?
>>>>>> 
>>>>>> It doesn't mean "low" and "high", though, right?  It means
>>>>>> "count"/"size" and "index of first element”.
>>>>> 
>>>>> You are probably right. I was thinking of DWARF where a subrange may either have a
>>>>> DW_AT_lower_bound and/or a DW_AT_upper_bound
>>>>> or a
>>>>> DW_AT_lower_bound and/or a DW_AT_count
>>>>> 
>>>>> We only support the second form so count and lower_bound would probably be most accurate.
>>>> 
>>>> Sounds good.  In keeping with the field naming convention I've adopted,
>>>> I'll use 'count:' and 'lowerBound:’.
>>> 
>>> yes, sure!
>>>> 
>>>>>> 
>>>>>> Or have I misinterpreted it?
>>>>>> 
>>>>>> How about "count" and "low"?
>>>>>> 
>>>>>>> 
>>>>>>> - do we still need the uniqueID on MDLexicalBlock? Could it be a distinct node instead?
>>>>>> 
>>>>>> I think this should be distinct instead.  I was planning to do this
>>>>>> as part of the upgrade (moving them into place).
>>>>>> 
>>>>>>> 
>>>>>>> - is it possible to have it print DW_OP-* constants such as in
>>>>>>> !MDExpression(DW_OP_deref, DW_OP_bit_piece, 0, 8)
>>>>>>> ?
>>>>>> 
>>>>>> I think this is pretty difficult the way that expressions are stored
>>>>>> right now -- it requires knowing how many arguments each thing takes,
>>>>>> etc.  In particular, how would the `AsmWriter` know whether something
>>>>>> should have a symbolic constant (`DW_OP_deref`) vs. being the number 6?
>>>>> 
>>>>> It would have to reuse the code from DIExpression::printInternal().
>>>> 
>>>> I think this is doable.
>>>> 
>>>> On the LLParser side, I've generally allowed raw ints (for `DW_TAG*`,
>>>> `DW_LANG*`, etc.), but for `DW_OP`s we could (at least for now) require
>>>> symbolic constants.
>>>> 
>>>>>> 
>>>>>> I was thinking of trying to clean this up later, but since we're
>>>>>> talking about it:
>>>>>> 
>>>>>> I think instead of `std::vector<uint64_t>` it should be
>>>>>> `std::vector<ExprOperand>`, where:
>>>>>> 
>>>>>> struct ExprOperand {
>>>>>> LocationAtom Kind;
>>>>>> unsigned NumArgs;
>>>>>> uint64_t Args[2];
>>>>>> ExprOperand(LocationAtom Kind);
>>>>>> ExprOperand(LocationAtom Kind, uint64_t Arg);
>>>>>> ExprOperand(LocationAtom Kind, uint64_t Arg1, uint64_t Arg2);
>>>>>> 
>>>>>> static ExprOperand getDeref() {
>>>>>>   return ExprOperand(dwarf::DW_OP_deref);
>>>>>> }
>>>>>> static ExprOperand getPiece(uint64_t A1, uint64_t A2) {
>>>>>>   return ExprOperand(dwarf::DW_OP_bit_piece, A1, A2);
>>>>>> }
>>>>>> };
>>>>>> 
>>>>>> ExprOperand Ops = {
>>>>>> ExprOperand::getDeref(),
>>>>>> ExprOperand::getPiece(0, 8)
>>>>>> };
>>>>>> auto *Expr = MDExpression::get(Ops);
>>>>>> 
>>>>>> Then the above could trivially be pretty-printed/parsed as:
>>>>>> 
>>>>>> !MDExpression(DW_OP_deref, DW_OP_bit_piece(0, 8))
>>>>> 
>>>>> or even better ... DW_OP_bit_piece(offset: 0, size: 8)
>>>> 
>>>> Sure.
>>>> 
>>>>> Yeah that looks good. Also we would need to introduce DW_OP_constu and get rid of the pseudo argument that DW_OP_plus currently takes.
>>>> 
>>>> Not necessary.  You can represent this already with:
>>>> 
>>>> DW_OP_plus(7)
>>>> 
>>>> where `7` is the pseudo-argument.  This is somewhat horrible, but
>>>> so is `DW_OP_plus` taking a pseudo-argument ;).
>>>> 
>>>>> This doesn’t need to happen right away, but we should keep it in mind.
>>>> 
>>>> I think something like this would simplify the logic a lot, besides
>>>> making it more extensible.
>>>> 
>>>>>> 
>>>>>> Thoughts?
>>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> Will it still print the old-style comments that testcases tend to match on, or are they obsoleted by the human-readable syntax?
>>>>>> 
>>>>>> They're obsoleted.  The old-style comments will get stale and just
>>>>>> add noise.  If we've regressed somehow let me know how.
>>>>> 
>>>>> See my comment on DW_OP_bit_piece above :-)
>>>>> 
>>>> 
>>>> Hah, right, right :).
>>>> 
>>>> I'll see if I can get something together for you.  If not we can keep
>>>> the DW_TAG_expression comments temporarily.
>>>> 
>>>> Is it alright to lose the "offset=" and "size=" markers (at least
>>>> temporarily)?  I don't see a natural place to put them in the syntax.
>>>> E.g., I was thinking I'd aim for the following for now:
>>>> 
>>>> !MDExpression(DW_OP_deref, DW_OP_plus, 8, DW_OP_bit_piece, 0, 8)
>>>> 
>>>> (and with no comment).  Is this sufficient?
>>> 
>>> Sufficient as an intermediate step on our way to the above expression being written as
>>> 
>>> !MDExpression(DW_OP_deref, DW_OP_constu(8), DW_OP_plus, DW_OP_bit_piece(offset: 0, size: 8))
>> 
>> Yup, although I'm happy for someone else to drive that -- the biggest
>> change will be switching to `DW_OP_constu`.  Once that's done I can
>> do the assembly if you want.
>> 
>> Attached a new set of patches (included the full series for reference).
>> The ones you probably want to look at are:
>> 
>> - 0001/0002: getOperationEncoding().
>> - 0003: Porting the DIExpression logic over.  I did a few things
>>   differently since the base iterator is random access.
>> - 0021: MDExpression.
> 
> All 4 look good to me.

Okay, those 4 (plus the others you'd already okay'ed) were committed in:

r228967, r229000, r229001, r229002, r229003, r229004, r229005, r229006,
r229007, r229009, r229010, r229011, r229013, r229014, r229015, r229016,
r229017, r229018, r229019, r229020, r229022, r229023, r229024.

I've been taken to task (over IRC) on my terse commit messages -- sorry
all for brevity.

> I hope you’ll be able to guide me trough the DW_OP_constu change later?
> 
> We should add a new coding guideline: If you touch llvm.vim you must also update llvm-mode.el and vice versa ;-)
> 
> -- adrian
> 
>> 
>> I'll start pushing through the others while you take a look at the
>> changes.
>> 
>> <all.patch>
>> 
>> <0001-Support-Rewrite-LocationAtom-and-OperationEncodingSt.patch><0002-Support-Add-dwarf-getOperationEncoding.patch><0003-IR-Add-MDExpression-ExprOperand.patch><0004-AsmWriter-Bitcode-MDSubrange.patch><0005-AsmWriter-Bitcode-MDEnumerator.patch><0006-AsmWriter-Bitcode-MDBasicType.patch><0007-AsmWriter-MDBasicType-Recognize-DW_ATE-in-encoding.patch><0008-AsmWriter-Bitcode-MDFile.patch><0009-AsmWriter-Bitcode-MDDerivedType-and-MDCompositeType.patch><0010-AsmWriter-MDCompositeType-Recognize-DW_LANG-in-runti.patch><0011-AsmWriter-Bitcode-MDSubroutineType.patch><0012-AsmWriter-Bitcode-MDCompileUnit.patch><0013-AsmWriter-Bitcode-MDSubprogram.patch><0014-AsmWriter-MDSubprogram-Recognize-DW_VIRTUALITY-in-vi.patch><0015-AsmWriter-Bitcode-MDLexicalBlock.patch><0016-AsmWriter-Bitcode-MDLexicalBlockFile.patch><0017-AsmWriter-Bitcode-MDNamespace.patch><0018-AsmWriter-Bitcode-MDTemplate-Type-Value-Parameter.patch><0019-AsmWriter-Bitcode-MDGlobalVariable.patch><0020-AsmWriter-Bitcode-MDLocalVariable.patch><0021-AsmWriter-Bitcode-MDExpression.patch><0022-AsmWriter-Bitcode-MDObjCProperty.patch><0023-AsmWriter-Bitcode-MDImportedEntity.patch>