<div dir="ltr"><br><br><div class="gmail_quote">On Mon Feb 02 2015 at 2:21:03 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015-Feb-02, at 13:51, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
><br>
><br>
>> On Feb 2, 2015, at 1:34 PM, Frédéric Riss <<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>> wrote:<br>
>><br>
>><br>
>>> On Feb 2, 2015, at 12:55 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>>><br>
>>> Add assembly/bitcode support to `GenericDebugNode`.  There are two<br>
>>> things I need reviewed here (although other comments are obviously<br>
>>> welcome):<br>
>>><br>
>>> - The first three patches add a `dwarf::getTag()` to complement<br>
>>>  `dwarf::TagString()`.  There's a small functionality change in the<br>
>>>  first patch I want confirmation on: `dwarf::TagString()` will return<br>
>>>  `nullptr` for `DW_TAG_lo_user` (and related non-tags).  This<br>
>>>  constant is not really a DWARF tag; it's just a marker for the first<br>
>>>  recommended user-defined tag).  Stringifying it seems wrong (and<br>
>>>  would conflict with stringifying a user-defined tag of the same<br>
>>>  number).<br>
>><br>
>> The 3 patches SGTM, a couple of questions/remarks though:<br>
>><br>
>> I agree that stringifying lo_user and friends makes no real sense as we<br>
>> are using the TagString function to pretty print actual dwarf data. Not that<br>
>> it matters today as we have no such extension registered.<br>
><br>
>><br>
>> Why would you want to ever accept them as input? I really don’t see a<br>
>> reason, but given you’ve introduced a special case for them, you really<br>
>> must need them.<br>
><br>
> Derived tools like llvm-dwarfdump should be able to handle DWARF generated by other compilers and should probably print something like DW_TAG_user(0x1234) when it encounters an unknown tag.<br>
<br>
That makes sense, but it doesn't affect `getTag()`, since that's<br></blockquote><div><br></div><div>Agreed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
unstringifying things.  And it doesn't seem to fit into `TagString()` too<br>
well either.  I think that logic should probably live in llvm-dwarfdump.<br>
<br></blockquote><div><br></div><div>Don't agree :)</div><div><br></div><div>All of the internal debugging interfaces use TagString as well.</div><div><br></div><div>Mostly a side note though, LGTM.</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> -- adrian<br>
>><br>
>> Is there a way to cleanly generate the enum in the Dwarf.h file from the<br>
>> .def file also? This way there would be only one source of tag definitions.<br>
>> (This would me having Dwarf.def as an exported header, which is quite<br>
>> ugly). At least add a comment to the header that anything added to the<br>
>> enum should be added to the def file.<br>
>><br>
>> Fred<br>
>><br>
>>> - The fourth patch adds assembly/bitcode support, and the fifth<br>
>>>  improves it by using `dwarf::getTag()`.  Here's the resulting<br>
>>>  syntax:<br>
>>><br>
>>>      !0 = !GenericDebugNode(tag: DW_TAG_entry_point,<br>
>>>                             header: "some\00header",<br>
>>>                             operands: {!1, !2, !7})<br>
>>><br>
>>>  This shouldn't be surprising (or a departure from the plan of<br>
>>>  record), but officially I need every assembly change reviewed<br>
>>>  pre-commit.<br>
>>><br>
>>> Note that this commit doesn't actually move `GenericDebugNode` into<br>
>>> place (and that's not the next step; more below for those interested).<br>
>>><br>
>>> (An extended side-note:<br>
>>><br>
>>> I've been sitting on these patches for a while, kind of stumped on the<br>
>>> best way to stage the rest of the work here.<br>
>>><br>
>>> Originally, I'd planned to move `GenericDebugNode` in underneath the<br>
>>> `DIDescriptor` classes right after this patch series.  This would have<br>
>>> required updating all the testcases to use it, only to subsequently<br>
>>> update every line *again* as I implemented the more specialized nodes<br>
>>> (making schema changes along the way).<br>
>>><br>
>>> Instead I'll be proceeding as follows: add specialized nodes that match<br>
>>> the current schema, add assembly/bitcode support for them (I have<br>
>>> out-of-tree patches that get to here), move them all underneath the<br>
>>> `DIDescriptor` hierarchy (only requiring each line to be updated<br>
>>> *once*), and then start hacking the schema once they're in place.<br>
>>><br>
>>> The new approach minimizes testcase churn, and (perhaps more<br>
>>> importantly) better separates the infrastructure changes (which are<br>
>>> rather mechanical) from the schema changes (which need more careful<br>
>>> consideration).)<br>
>>><br>
>>> <0001-Support-Stop-<u></u>stringifying-DW_TAG_-lo-hi-_<u></u>user.patch><0002-Support-Re-<u></u>implement-dwarf-TagString-<u></u>using-a-.def-fi.patch><0003-<u></u>Support-Add-string-unsigned-<u></u>mapping-for-DW_TAG.patch><<u></u>0004-IR-Assembly-and-bitcode-<u></u>for-GenericDebugNode.patch><<u></u>0005-AsmParser-Recognize-DW_<u></u>TAG_-constants.patch><br>
>><br>
><br>
<br>
</blockquote></div></div>