[PATCH] Assembly/bitcode support for GenericDebugNode

[Frédéric Riss]

> 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

> (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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/05349ec3/attachment.html>