llvm dev - Feb 2015 - [LLVMdev] Questions before moving the new debug info hierarchy into place

I'm getting close to executing the transition to the new debug info
hierarchy.  For reference, I've attached two WIP patches (which would be
squashed before commit) and the WIP upgrade script I'm using.

    - transition-code.patch: Change the `DIDescriptor` hierarchy to
      lightweight wrappers around the subclasses of `DebugNode` (instead
      of rather heavier wrappers around `MDTuple`), and update
      `DIBuilder` to generate the right things.
    - transition-testcases.patch: Upgrade of testcases, entirely
      generated from the upgrade script (so far).
    - upgrade-specialized-nodes.sh: Script to upgrade LLVM assembly.

(Feel free to have a look, but I haven't updated LangRef, I don't quite
have `make check` passing, and I haven't even started on `make
check-clang` (I imagine that'll all be done by hand).)

There are two outstanding issues I'd like some feedback on.

Pretty-printing the flags
========================
I've noticed the `flags:` field is *harder* to read in the new assembly:

    !MDDerivedType(flags: 16384, ...)

than the pretty-printed comments in the old:

    !{!"...\\0016384", ...} ; ... [public] [rvalue reference]

I don't want to regress here.

In `DIDescriptor`, the flags are described in an enum bitfield:

    FlagAccessibility     = 1 << 0 | 1 << 1,
    FlagPrivate           = 1,
    FlagProtected         = 2,
    FlagPublic            = 3,
    FlagFwdDecl           = 1 << 2,
    FlagAppleBlock        = 1 << 3,
    FlagBlockByrefStruct  = 1 << 4,
    FlagVirtual           = 1 << 5,
    FlagArtificial        = 1 << 6,
    FlagExplicit          = 1 << 7,
    FlagPrototyped        = 1 << 8,
    FlagObjcClassComplete = 1 << 9,
    FlagObjectPointer     = 1 << 10,
    FlagVector            = 1 << 11,
    FlagStaticMember      = 1 << 12,
    FlagLValueReference   = 1 << 13,
    FlagRValueReference   = 1 << 14

I think the right short-term solution is to use these names directly in
assembly, giving us:

    !MDDerivedType(flags: FlagPublic | FlagRValueReference, ...)

This is easy to implement and easy to CHECK against.

Sound good?

(Eventually, I'd like to use the `DW_AT` symbols that each of these
corresponds to, but `FlagStaticMember` doesn't seem to line up with any
such `DW_AT` so that will take some refactoring (and I don't think it
makes sense for that to block moving the hierarchy into place).)

Merging the two types of files
=============================
In the old format, we have two types of files -- an untagged file node,
and a DIFile/DW_TAG_file_type/0x29 which references the untagged node.

    !0 = !{!"path/to/file", !"/path/to/dir"}
    !1 = !{!"0x29", !0}

In the actual metadata graph, most file references use !0, but in
DIBuilder !1 is always passed in and the !0 is extracted from it.

I've been planning to merge these into:

    !1 = !MDFile(filename: "path/to/file", directory:
"/path/to/dir")

Anyone see a problem with that?  Why?

If not, I'd like to roll that change into the same patch in order to
reduce testcase churn.  (I've discovered that it won't complicate the
code patch at all.)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: transition-code.patch
Type: application/octet-stream
Size: 325 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150219/57938fb3/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: transition-testcases.patch
Type: application/octet-stream
Size: 1754066 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150219/57938fb3/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: upgrade-specialized-nodes.sh
Type: application/octet-stream
Size: 49628 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150219/57938fb3/attachment-0002.obj>
-------------- next part --------------

> On Feb 19, 2015, at 7:49 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> 
> I'm getting close to executing the transition to the new debug info
> hierarchy.  For reference, I've attached two WIP patches (which would
be
> squashed before commit) and the WIP upgrade script I'm using.
> 
>    - transition-code.patch: Change the `DIDescriptor` hierarchy to
>      lightweight wrappers around the subclasses of `DebugNode` (instead
>      of rather heavier wrappers around `MDTuple`), and update
>      `DIBuilder` to generate the right things.
>    - transition-testcases.patch: Upgrade of testcases, entirely
>      generated from the upgrade script (so far).
>    - upgrade-specialized-nodes.sh: Script to upgrade LLVM assembly.
> 
> (Feel free to have a look, but I haven't updated LangRef, I don't
quite
> have `make check` passing, and I haven't even started on `make
> check-clang` (I imagine that'll all be done by hand).)
> 
> There are two outstanding issues I'd like some feedback on.
> 
> Pretty-printing the flags
> ========================> 
> I've noticed the `flags:` field is *harder* to read in the new
assembly:
> 
>    !MDDerivedType(flags: 16384, ...)
> 
> than the pretty-printed comments in the old:
> 
>    !{!"...\\0016384", ...} ; ... [public] [rvalue reference]
> 
> I don't want to regress here.
> 
> In `DIDescriptor`, the flags are described in an enum bitfield:
> 
>    FlagAccessibility     = 1 << 0 | 1 << 1,
>    FlagPrivate           = 1,
>    FlagProtected         = 2,
>    FlagPublic            = 3,
>    FlagFwdDecl           = 1 << 2,
>    FlagAppleBlock        = 1 << 3,
>    FlagBlockByrefStruct  = 1 << 4,
>    FlagVirtual           = 1 << 5,
>    FlagArtificial        = 1 << 6,
>    FlagExplicit          = 1 << 7,
>    FlagPrototyped        = 1 << 8,
>    FlagObjcClassComplete = 1 << 9,
>    FlagObjectPointer     = 1 << 10,
>    FlagVector            = 1 << 11,
>    FlagStaticMember      = 1 << 12,
>    FlagLValueReference   = 1 << 13,
>    FlagRValueReference   = 1 << 14
> 
> I think the right short-term solution is to use these names directly in
> assembly, giving us:
> 
>    !MDDerivedType(flags: FlagPublic | FlagRValueReference, ...)
> 
> This is easy to implement and easy to CHECK against.
> 
> Sound good?
> 
> (Eventually, I'd like to use the `DW_AT` symbols that each of these
> corresponds to, but `FlagStaticMember` doesn't seem to line up with any
> such `DW_AT` so that will take some refactoring (and I don't think it
> makes sense for that to block moving the hierarchy into place).)
I support this proposal under the condition that it is only an intermediate
step.

What I would like to evolve this to is a more generic infrastructure were we
have a list of “trivial" attributes (= attributes that don’t need any
special backend support) so we could write things like
   MDCompositeType(attributes: [DW_AT_artificial, DW_AT_rvalue_reference,
DW_AT_public], ...)

perhaps even
   MDCompositeType(attributes:
[DW_AT_vtable_elem_location(DIExpression(DW_OP_constu(3)))], ...)
?

[ Looks nice, but it isn’t perfect, of course. For example:
DW_AT_rvalue_reference is technically only available in DWARF5. Should the
frontend then have to know/care about these details, or should the backend
perform a lowering from a DWARF5-based IR to whatever the module flags
requested? ]

-- adrian

> 
> Merging the two types of files
> =============================> 
> In the old format, we have two types of files -- an untagged file node,
> and a DIFile/DW_TAG_file_type/0x29 which references the untagged node.
> 
>    !0 = !{!"path/to/file", !"/path/to/dir"}
>    !1 = !{!"0x29", !0}
> 
> In the actual metadata graph, most file references use !0, but in
> DIBuilder !1 is always passed in and the !0 is extracted from it.
> 
> I've been planning to merge these into:
> 
>    !1 = !MDFile(filename: "path/to/file", directory:
"/path/to/dir")
> 
> Anyone see a problem with that?  Why?
> 
> If not, I'd like to roll that change into the same patch in order to
> reduce testcase churn.  (I've discovered that it won't complicate
the
> code patch at all.)
> 
>
<transition-code.patch><transition-testcases.patch><upgrade-specialized-nodes.sh>
>

I agree with Adrian here, and SGTM.

Thanks for all of your very hard work Duncan!

-eric

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

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

I take it you'll have other constructs (I think all scopes have a
file/directory) reference the MDFile directly?

>
> If not, I'd like to roll that change into the same patch in order to
> reduce testcase churn.  (I've discovered that it won't complicate
the
> code patch at all.)
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150220/6e2363b2/attachment.html>

> On Feb 20, 2015, at 9:00 AM, David Blaikie <dblaikie at gmail.com>
wrote:
> 
> 
> 
> On Thu, Feb 19, 2015 at 7:49 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com <mailto:dexonsmith at apple.com>> wrote:
> I'm getting close to executing the transition to the new debug info
> hierarchy.  For reference, I've attached two WIP patches (which would
be
> squashed before commit) and the WIP upgrade script I'm using.
> 
>     - transition-code.patch: Change the `DIDescriptor` hierarchy to
>       lightweight wrappers around the subclasses of `DebugNode` (instead
>       of rather heavier wrappers around `MDTuple`), and update
>       `DIBuilder` to generate the right things.
>     - transition-testcases.patch: Upgrade of testcases, entirely
>       generated from the upgrade script (so far).
>     - upgrade-specialized-nodes.sh: Script to upgrade LLVM assembly.
> 
> (Feel free to have a look, but I haven't updated LangRef, I don't
quite
> have `make check` passing, and I haven't even started on `make
> check-clang` (I imagine that'll all be done by hand).)
> 
> There are two outstanding issues I'd like some feedback on.
> 
> Pretty-printing the flags
> ========================> 
> I've noticed the `flags:` field is *harder* to read in the new
assembly:
> 
>     !MDDerivedType(flags: 16384, ...)
> 
> than the pretty-printed comments in the old:
> 
>     !{!"...\\0016384", ...} ; ... [public] [rvalue reference]
> 
> I don't want to regress here.
> 
> In `DIDescriptor`, the flags are described in an enum bitfield:
> 
>     FlagAccessibility     = 1 << 0 | 1 << 1,
>     FlagPrivate           = 1,
>     FlagProtected         = 2,
>     FlagPublic            = 3,
>     FlagFwdDecl           = 1 << 2,
>     FlagAppleBlock        = 1 << 3,
>     FlagBlockByrefStruct  = 1 << 4,
>     FlagVirtual           = 1 << 5,
>     FlagArtificial        = 1 << 6,
>     FlagExplicit          = 1 << 7,
>     FlagPrototyped        = 1 << 8,
>     FlagObjcClassComplete = 1 << 9,
>     FlagObjectPointer     = 1 << 10,
>     FlagVector            = 1 << 11,
>     FlagStaticMember      = 1 << 12,
>     FlagLValueReference   = 1 << 13,
>     FlagRValueReference   = 1 << 14
> 
> I think the right short-term solution is to use these names directly in
> assembly, giving us:
> 
>     !MDDerivedType(flags: FlagPublic | FlagRValueReference, ...)
> 
> This is easy to implement and easy to CHECK against.
> 
> Sound good?
> 
> (Eventually, I'd like to use the `DW_AT` symbols that each of these
> corresponds to, but `FlagStaticMember` doesn't seem to line up with any
> such `DW_AT` so that will take some refactoring (and I don't think it
> makes sense for that to block moving the hierarchy into place).)
> 
> Merging the two types of files
> =============================> 
> In the old format, we have two types of files -- an untagged file node,
> and a DIFile/DW_TAG_file_type/0x29 which references the untagged node.
> 
>     !0 = !{!"path/to/file", !"/path/to/dir"}
>     !1 = !{!"0x29", !0}
> 
> In the actual metadata graph, most file references use !0, but in
> DIBuilder !1 is always passed in and the !0 is extracted from it.
> 
> I've been planning to merge these into:
> 
>     !1 = !MDFile(filename: "path/to/file", directory:
"/path/to/dir")
> 
> Anyone see a problem with that?  Why?
> 
> If the strings are deduplicated elsewhere, that should be fine. (I think I
made the change originally to pull out the names because of the duplication I
saw in many constructsn needing to reference the file/directory and they all
contained the same file/directory)
No specific comments on the approach which seems fine. One question though:

Speaking of deduplication of filenames. I think we discussed that in the early
stages of your work, but I just wanted to make sure I remember correctly: the
new debug hierarchy will allow to implement specific uniquing behavior, right?
So we will be able to tweak MDFile to unique:

!MDFile(filename: "path/to/file", directory: "/path/to/dir")
!MDFile(filename: "to/file", directory: "/path/to/dir/path")

into the same object? I did some work in this area and in the current form it’s
not really possible to do cleanly (don’t remember the details here, but I know
punted it till we get smart uniquing capability).

Thanks!
Fred
> I take it you'll have other constructs (I think all scopes have a
file/directory) reference the MDFile directly?
>  
> 
> If not, I'd like to roll that change into the same patch in order to
> reduce testcase churn.  (I've discovered that it won't complicate
the
> code patch at all.)
> 
> 
> 
> 
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150220/65114a9b/attachment.html>

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

> On 2015-Feb-20, at 09:00, David Blaikie <dblaikie at gmail.com>
wrote:
> 
> 
> 
> On Thu, Feb 19, 2015 at 7:49 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> I'm getting close to executing the transition to the new debug info
> hierarchy.  For reference, I've attached two WIP patches (which would
be
> squashed before commit) and the WIP upgrade script I'm using.
> 
>     - transition-code.patch: Change the `DIDescriptor` hierarchy to
>       lightweight wrappers around the subclasses of `DebugNode` (instead
>       of rather heavier wrappers around `MDTuple`), and update
>       `DIBuilder` to generate the right things.
>     - transition-testcases.patch: Upgrade of testcases, entirely
>       generated from the upgrade script (so far).
>     - upgrade-specialized-nodes.sh: Script to upgrade LLVM assembly.
> 
> (Feel free to have a look, but I haven't updated LangRef, I don't
quite
> have `make check` passing, and I haven't even started on `make
> check-clang` (I imagine that'll all be done by hand).)
> 
> There are two outstanding issues I'd like some feedback on.
> 
> Pretty-printing the flags
> ========================> 
> I've noticed the `flags:` field is *harder* to read in the new
assembly:
> 
>     !MDDerivedType(flags: 16384, ...)
> 
> than the pretty-printed comments in the old:
> 
>     !{!"...\\0016384", ...} ; ... [public] [rvalue reference]
> 
> I don't want to regress here.
> 
> In `DIDescriptor`, the flags are described in an enum bitfield:
> 
>     FlagAccessibility     = 1 << 0 | 1 << 1,
>     FlagPrivate           = 1,
>     FlagProtected         = 2,
>     FlagPublic            = 3,
>     FlagFwdDecl           = 1 << 2,
>     FlagAppleBlock        = 1 << 3,
>     FlagBlockByrefStruct  = 1 << 4,
>     FlagVirtual           = 1 << 5,
>     FlagArtificial        = 1 << 6,
>     FlagExplicit          = 1 << 7,
>     FlagPrototyped        = 1 << 8,
>     FlagObjcClassComplete = 1 << 9,
>     FlagObjectPointer     = 1 << 10,
>     FlagVector            = 1 << 11,
>     FlagStaticMember      = 1 << 12,
>     FlagLValueReference   = 1 << 13,
>     FlagRValueReference   = 1 << 14
> 
> I think the right short-term solution is to use these names directly in
> assembly, giving us:
> 
>     !MDDerivedType(flags: FlagPublic | FlagRValueReference, ...)
> 
> This is easy to implement and easy to CHECK against.
> 
> Sound good?
> 
> (Eventually, I'd like to use the `DW_AT` symbols that each of these
> corresponds to, but `FlagStaticMember` doesn't seem to line up with any
> such `DW_AT` so that will take some refactoring (and I don't think it
> makes sense for that to block moving the hierarchy into place).)
> 
> Merging the two types of files
> =============================> 
> In the old format, we have two types of files -- an untagged file node,
> and a DIFile/DW_TAG_file_type/0x29 which references the untagged node.
> 
>     !0 = !{!"path/to/file", !"/path/to/dir"}
>     !1 = !{!"0x29", !0}
> 
> In the actual metadata graph, most file references use !0, but in
> DIBuilder !1 is always passed in and the !0 is extracted from it.
> 
> I've been planning to merge these into:
> 
>     !1 = !MDFile(filename: "path/to/file", directory:
"/path/to/dir")
> 
> Anyone see a problem with that?  Why?
> 
> If the strings are deduplicated elsewhere, that should be fine. (I think I
made the change originally to pull out the names because of the duplication I
saw in many constructsn needing to reference the file/directory and they all
contained the same file/directory)
Yup, they'll be de-duped via `MDFile`.
> I take it you'll have other constructs (I think all scopes have a
file/directory) reference the MDFile directly?
I'll make it so all `file:` references point to an `MDFile`; so
far this is only true for `MDCompileUnit` (the others point to
the untagged node).  There are some places (at least in
testcases) where `MDFile` is being used in a `scope:` reference;
I'll leave those unchanged (although IIUC, Adrian thinks a file
should never be used as a scope -- you always have a better
option (compile unit, namespace, etc.) -- so maybe that'll be
changing eventually... we just need be mindful of type uniquing).
>  
> 
> If not, I'd like to roll that change into the same patch in order to
> reduce testcase churn.  (I've discovered that it won't complicate
the
> code patch at all.)