[PATCH] IR: Make MDNode::dump() useful by adding addresses

[Duncan P. N. Exon Smith]

> On 2014 Dec 12, at 16:32, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Dec 12, 2014 at 4:09 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2014 Dec 12, at 15:47, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Fri, Dec 12, 2014 at 2:37 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > It's horrible to inspect `MDNode`s in a debugger.  All of their operands
> > that are `MDNode`s get dumped as `<badref>`, since we can't assign
> > metadata slots in the context of a `Metadata::dump()`.  (Why not?  Why
> > not assign numbers lazily?  Because then each time you called `dump()`,
> > a given `MDNode` could have a different lazily assigned number.)
> >
> > Fortunately, the C memory model gives us perfectly good identifiers for
> > `MDNode`.  Add pointer addresses to the dumps, transforming this:
> >
> >     (lldb) e N->dump()
> >     metadata !{i32 662302, i32 26, metadata <badref>, null}
> >
> >     (lldb) e ((MDNode*)N->getOperand(2))->dump()
> >     metadata !{i32 4, metadata !"foo"}
> >
> > into:
> >
> >     (lldb) e N->dump()
> >     metadata !{i32 662302, i32 26, metadata <badref:0x100706ee0>, null}
> >
> >     (lldb) e ((MDNode*)0x100706ee0)->dump()
> >     metadata !{i32 4, metadata !"foo"}
> >
> > and this:
> >
> >     (lldb) e N->dump()
> >     metadata !{metadata <badref>, metadata <badref>, metadata <badref>, metadata <badref>, metadata <badref>}
> >
> >     (lldb) e N->getOperand(0)
> >     (const llvm::MDOperand) $0 = {
> >       MD = 0x00000001012004e0
> >     }
> >     (lldb) e N->getOperand(1)
> >     (const llvm::MDOperand) $1 = {
> >       MD = 0x00000001012004e0
> >     }
> >     (lldb) e N->getOperand(2)
> >     (const llvm::MDOperand) $2 = {
> >       MD = 0x0000000101200058
> >     }
> >     (lldb) e N->getOperand(3)
> >     (const llvm::MDOperand) $3 = {
> >       MD = 0x00000001012004e0
> >     }
> >     (lldb) e N->getOperand(4)
> >     (const llvm::MDOperand) $4 = {
> >       MD = 0x0000000101200058
> >     }
> >     (lldb) e ((MDNode*)0x00000001012004e0)->dump()
> >     metadata !{}
> >
> >     (lldb) e ((MDNode*)0x0000000101200058)->dump()
> >     metadata !{null}
> >
> > into:
> >
> >     (lldb) e N->dump()
> >     metadata !{metadata <badref:0x1012004e0>, metadata <badref:0x1012004e0>, metadata <badref:0x101200058>, metadata <badref:0x1012004e0>, metadata <badref:0x101200058>}
> >
> >     (lldb) e ((MDNode*)0x1012004e0)->dump()
> >     metadata !{}
> >
> >     (lldb) e ((MDNode*)0x101200058)->dump()
> >     metadata !{null}
> >
> > I've left `badref` in the operand to keep it `FileCheck`-able, via:
> >
> >     CHECK-NOT: badref
> >
> > badref seems like a pretty weird anachronism & I don't see any instances of "CHECK.*badref" in 'test' (but it could be under other prefixes)
> >
> >
> > Another option would be:
> >
> >     (lldb) e N->dump()
> >     metadata !{metadata !0x1012004e0, metadata !0x1012004e0, metadata !0x101200058, metadata !0x1012004e0, metadata !0x101200058}
> >
> > which would allow:
> >
> >     CHECK-NOT: !0x
> >
> > I'd just imagine you'd check for "{.*metadata!" - which will be harder in your changes, but still possible I would imagine?
> >
> >
> > but IMO that's too subtle...
> >
> > Not sure - seems almost less confusing than having 'badref' around.
> >
> 
> `badref` is used consistently for `Value`s that don't have an assigned slot,
> but now that `Metadata` is separate it can probably do its own thing.
> 
> There is only once use in LLVM, not sure about clang:
> 
>     $ git grep badref
>     Assembler/2010-02-05-FunctionLocalMetadataBecomesNull.ll:; RUN: opt -O3 < %s | llvm-dis | not grep badref
> 
> But this test should probably just CHECK for the expected result anyway.
> 
> I'm pretty much happy with anything that gives a pointer address.  Here are
> some paint colours:
> 
>     !{!0, null, <badref>}
>     !{!0, null, 0xabc}
>     !{!0, null, !0xabc}
>     !{!0, null, <0xabc>}
>     !{!0, null, !<0xabc>}
>     !{!0, null, <badref:0xabc>}
> 
> I'd prefer not the 'badref' versions, and as you say, verbosity is a concern so that tends towards the more brief versions... 
>  
> 
> Everyone okay with `!{!0, null, <0xabc>}`?
> 
> I'm OK with that.
> 
> I kind of thought of ! as being the prefix for a value/metadata reference, but it does seem that the ! is sort of part of the numbered/named metadata, so it shouldn't necessarily appear for these address references. *shrug*

Committed in r224325.