[PATCH] Assembly/bitcode support for GenericDebugNode

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Feb 4 13:26:52 PST 2015


> On 2015-Feb-02, at 16:48, Eric Christopher <echristo at gmail.com> wrote:
> 
> 
> 
> On Mon Feb 02 2015 at 2:21:03 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2015-Feb-02, at 13:51, Adrian Prantl <aprantl at apple.com> wrote:
> >
> >
> >> On Feb 2, 2015, at 1:34 PM, 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.
> >
> > 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.
> 
> That makes sense, but it doesn't affect `getTag()`, since that's
> 
> Agreed.
>  
> unstringifying things.  And it doesn't seem to fit into `TagString()` too
> well either.  I think that logic should probably live in llvm-dwarfdump.
> 
> 
> Don't agree :)
> 
> All of the internal debugging interfaces use TagString as well.
> 
> Mostly a side note though, LGTM.

Thanks for the review!  Committed as r228029, r228030, r228031, r228041
and r228042 (with a clang fixup in the middle at r228032).



More information about the llvm-commits mailing list