[PATCH] Assembly/bitcode support for GenericDebugNode
Frédéric Riss
friss at apple.com
Mon Feb 2 13:56:59 PST 2015
> 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>
More information about the llvm-commits
mailing list