[PATCH] AsmWriter/Bitcode: Support specialized debug nodes
Eric Christopher
echristo at gmail.com
Thu Feb 12 16:21:59 PST 2015
All 4 look fine to me as well.
And yes, update llvm-mode.el and/or llvm.vim ;)
Thanks!
-eric
On Thu Feb 12 2015 at 4:18:17 PM 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.
> 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>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150213/2cf65fb7/attachment.html>
More information about the llvm-commits
mailing list