[PATCH] AsmWriter/Bitcode: Support specialized debug nodes
Adrian Prantl
aprantl at apple.com
Thu Feb 12 16:18:15 PST 2015
> 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.
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>
More information about the llvm-commits
mailing list