[PATCH] Assembly/bitcode support for GenericDebugNode

[Duncan P. N. Exon Smith]

> On 2015-Feb-02, at 13:56, Frédéric Riss <friss at apple.com> wrote:
> 
> 
>> On Feb 2, 2015, at 1:43 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>>> 
>>> On 2015 Feb 2, at 13:34, Frédéric Riss <friss at apple.com> wrote:
>>> 
>>>> 
>>>> On Feb 2, 2015, at 12:55 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>> Add assembly/bitcode support to `GenericDebugNode`.  There are two
>>>> things I need reviewed here (although other comments are obviously
>>>> welcome):
>>>> 
>>>> - The first three patches add a `dwarf::getTag()` to complement
>>>>  `dwarf::TagString()`.  There's a small functionality change in the
>>>>  first patch I want confirmation on: `dwarf::TagString()` will return
>>>>  `nullptr` for `DW_TAG_lo_user` (and related non-tags).  This
>>>>  constant is not really a DWARF tag; it's just a marker for the first
>>>>  recommended user-defined tag).  Stringifying it seems wrong (and
>>>>  would conflict with stringifying a user-defined tag of the same
>>>>  number).
>>> 
>>> The 3 patches SGTM, a couple of questions/remarks though:
>>> 
>>> I agree that stringifying lo_user and friends makes no real sense as we
>>> are using the TagString function to pretty print actual dwarf data. Not that
>>> it matters today as we have no such extension registered.
>>> 
>>> Why would you want to ever accept them as input? I really don’t see a
>>> reason, but given you’ve introduced a special case for them, you really
>>> must need them.
>> 
>> For completeness, that's all.  Unlike stringifying, there's a reasonable
>> answer in this case :).
>> 
>> I'll remove it if you don't think it's useful.
> 
> I don’t see how it would be useful (if someone used them in an input IR and
> then re-dumped this same IR, what would happen? I suppose you could
> have a fallback that uses raw number in that case…)
> 
>>> Is there a way to cleanly generate the enum in the Dwarf.h file from the
>>> .def file also? This way there would be only one source of tag definitions.
>>> (This would me having Dwarf.def as an exported header, which is quite
>>> ugly). At least add a comment to the header that anything added to the
>>> enum should be added to the def file.
>> 
>> I thought about this and it wouldn't be hard.  I'll resubmit with that
>> added.
> 
>> I originally decided against it because some of the DW_TAGs are defined
>> up in `LLVMConstants` instead of in the enum (namely, our made-up tags
>> called `DW_TAG_expression` and `DW_TAG_{auto,arg}_variable`).
>> 
>> If we can move those constants into the enum, then it shouldn't be a
>> problem.
> 
> Mmmh. I hadn’t thought of these. And of course you need to handle them
> as they are used in the debug nodes… They somewhat would ‘pollute’ the
> Tag enum with fake values. I don’t like that, but as a matter of fact we already
> have that pollution as we use the fake and real tags in the same context.
> 
> Fred
> 

FWIW, they were *already* handled in `TagString()`, so this doesn't
actually change any behaviour.  It just makes the ugliness easier to
spot.

I plan to expunge them anyway once I get to schema changes.  They're
only necessary since we had no other way of distinguishing them from
normal `MDNode`s.  Now that we have a class hierarchy, we don't need
these mock tags.

New patches attached.  (I'd appreciate an LGTM from someone on the
assembly changes too.)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-Stop-stringifying-DW_TAG_-lo-hi-_user.patch
Type: application/octet-stream
Size: 2960 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/eed6d113/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Support-Re-implement-dwarf-TagString-using-a-.def-fi.patch
Type: application/octet-stream
Size: 14411 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/eed6d113/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Support-Add-string-unsigned-mapping-for-DW_TAG.patch
Type: application/octet-stream
Size: 3073 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/eed6d113/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-IR-Assembly-and-bitcode-for-GenericDebugNode.patch
Type: application/octet-stream
Size: 16372 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/eed6d113/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-AsmParser-Recognize-DW_TAG_-constants.patch
Type: application/octet-stream
Size: 8858 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/eed6d113/attachment-0004.obj>
-------------- next part --------------

>> (My other hesitation was that `DW_TAG_invalid` is out of range for the
>> enum, but that's hardcoded *anyway*.)
>> 
>>> 
>>> Fred
>>> 
>>>> - The fourth patch adds assembly/bitcode support, and the fifth
>>>>  improves it by using `dwarf::getTag()`.  Here's the resulting
>>>>  syntax:
>>>> 
>>>>      !0 = !GenericDebugNode(tag: DW_TAG_entry_point,
>>>>                             header: "some\00header",
>>>>                             operands: {!1, !2, !7})
>>>> 
>>>>  This shouldn't be surprising (or a departure from the plan of
>>>>  record), but officially I need every assembly change reviewed
>>>>  pre-commit.
>>>> 
>>>> Note that this commit doesn't actually move `GenericDebugNode` into
>>>> place (and that's not the next step; more below for those interested).
>>>> 
>>>> (An extended side-note:
>>>> 
>>>> I've been sitting on these patches for a while, kind of stumped on the
>>>> best way to stage the rest of the work here.
>>>> 
>>>> Originally, I'd planned to move `GenericDebugNode` in underneath the
>>>> `DIDescriptor` classes right after this patch series.  This would have
>>>> required updating all the testcases to use it, only to subsequently
>>>> update every line *again* as I implemented the more specialized nodes
>>>> (making schema changes along the way).
>>>> 
>>>> Instead I'll be proceeding as follows: add specialized nodes that match
>>>> the current schema, add assembly/bitcode support for them (I have
>>>> out-of-tree patches that get to here), move them all underneath the
>>>> `DIDescriptor` hierarchy (only requiring each line to be updated
>>>> *once*), and then start hacking the schema once they're in place.
>>>> 
>>>> The new approach minimizes testcase churn, and (perhaps more
>>>> importantly) better separates the infrastructure changes (which are
>>>> rather mechanical) from the schema changes (which need more careful
>>>> consideration).)
>>>> 
>>>> <0001-Support-Stop-stringifying-DW_TAG_-lo-hi-_user.patch><0002-Support-Re-implement-dwarf-TagString-using-a-.def-fi.patch><0003-Support-Add-string-unsigned-mapping-for-DW_TAG.patch><0004-IR-Assembly-and-bitcode-for-GenericDebugNode.patch><0005-AsmParser-Recognize-DW_TAG_-constants.patch>
>