<div dir="ltr">I agree with Adrian here, and SGTM.<div><br></div><div>Thanks for all of your very hard work Duncan!</div><div><br></div><div>-eric<br><br><div class="gmail_quote">On Thu Feb 19 2015 at 7:49:39 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">I'm getting close to executing the transition to the new debug info<br>
hierarchy.  For reference, I've attached two WIP patches (which would be<br>
squashed before commit) and the WIP upgrade script I'm using.<br>
<br>
    - transition-code.patch: Change the `DIDescriptor` hierarchy to<br>
      lightweight wrappers around the subclasses of `DebugNode` (instead<br>
      of rather heavier wrappers around `MDTuple`), and update<br>
      `DIBuilder` to generate the right things.<br>
    - transition-testcases.patch: Upgrade of testcases, entirely<br>
      generated from the upgrade script (so far).<br>
    - upgrade-specialized-nodes.sh: Script to upgrade LLVM assembly.<br>
<br>
(Feel free to have a look, but I haven't updated LangRef, I don't quite<br>
have `make check` passing, and I haven't even started on `make<br>
check-clang` (I imagine that'll all be done by hand).)<br>
<br>
There are two outstanding issues I'd like some feedback on.<br>
<br>
Pretty-printing the flags<br>
=========================<br>
<br>
I've noticed the `flags:` field is *harder* to read in the new assembly:<br>
<br>
    !MDDerivedType(flags: 16384, ...)<br>
<br>
than the pretty-printed comments in the old:<br>
<br>
    !{!"...\\0016384", ...} ; ... [public] [rvalue reference]<br>
<br>
I don't want to regress here.<br>
<br>
In `DIDescriptor`, the flags are described in an enum bitfield:<br>
<br>
    FlagAccessibility     = 1 << 0 | 1 << 1,<br>
    FlagPrivate           = 1,<br>
    FlagProtected         = 2,<br>
    FlagPublic            = 3,<br>
    FlagFwdDecl           = 1 << 2,<br>
    FlagAppleBlock        = 1 << 3,<br>
    FlagBlockByrefStruct  = 1 << 4,<br>
    FlagVirtual           = 1 << 5,<br>
    FlagArtificial        = 1 << 6,<br>
    FlagExplicit          = 1 << 7,<br>
    FlagPrototyped        = 1 << 8,<br>
    FlagObjcClassComplete = 1 << 9,<br>
    FlagObjectPointer     = 1 << 10,<br>
    FlagVector            = 1 << 11,<br>
    FlagStaticMember      = 1 << 12,<br>
    FlagLValueReference   = 1 << 13,<br>
    FlagRValueReference   = 1 << 14<br>
<br>
I think the right short-term solution is to use these names directly in<br>
assembly, giving us:<br>
<br>
    !MDDerivedType(flags: FlagPublic | FlagRValueReference, ...)<br>
<br>
This is easy to implement and easy to CHECK against.<br>
<br>
Sound good?<br>
<br>
(Eventually, I'd like to use the `DW_AT` symbols that each of these<br>
corresponds to, but `FlagStaticMember` doesn't seem to line up with any<br>
such `DW_AT` so that will take some refactoring (and I don't think it<br>
makes sense for that to block moving the hierarchy into place).)<br>
<br>
Merging the two types of files<br>
==============================<br>
<br>
In the old format, we have two types of files -- an untagged file node,<br>
and a DIFile/DW_TAG_file_type/0x29 which references the untagged node.<br>
<br>
    !0 = !{!"path/to/file", !"/path/to/dir"}<br>
    !1 = !{!"0x29", !0}<br>
<br>
In the actual metadata graph, most file references use !0, but in<br>
DIBuilder !1 is always passed in and the !0 is extracted from it.<br>
<br>
I've been planning to merge these into:<br>
<br>
    !1 = !MDFile(filename: "path/to/file", directory: "/path/to/dir")<br>
<br>
Anyone see a problem with that?  Why?<br>
<br>
If not, I'd like to roll that change into the same patch in order to<br>
reduce testcase churn.  (I've discovered that it won't complicate the<br>
code patch at all.)<br>
<br>
<br>
<br>
</blockquote></div></div></div>