[llvm-commits] [PATCH] PR14471: Debug info for static data members (LLVM part)

[Eric Christopher]

This looks fine. Did you test outside of llvm? And do you need me to apply?

-eric


On Wed, Jan 9, 2013 at 4:55 PM, Robinson, Paul <Paul.Robinson at am.sony.com>wrote:

> Updated patch to address the review comments.
> --paulr
>
> ________________________________________
> From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-bounces at cs.uiuc.edu]
> on behalf of Robinson, Paul [Paul.Robinson at am.sony.com]
> Sent: Wednesday, January 09, 2013 3:22 PM
> To: Eric Christopher
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] PR14471: Debug info for static data
> members (LLVM part)
>
> Eric Christopher [echristo at gmail.com] wrote:
> > Hi Paul,
> >
> > Few comments:
> >
> > +; RUN: llc %s -o %t -filetype=obj -O0
> > +; RUN: llvm-dwarfdump %t | FileCheck %s -check-prefix=CHECK1
> > +; RUN: llvm-dwarfdump %t | FileCheck %s -check-prefix=CHECK2
> > +; PR14471
> >
> > Can we get a comment on the two checks? I see why (or at least think I
> > do) you've got them, but it's not necessarily obvious. I think it's
> > because it makes it easier for you to check the things you're looking
> for.
>
> Checking that some things do exist and other things do not exist in the
> same range of lines is inconvenient to express in FileCheck.  Easier to
> separate these into separate passes.  I do have a comment down where the
> actual CHECK lines are, but I can put one here too.
> And maybe use better names.
>
> > +
> > +; ModuleID = 'debug-info-static-member.cpp'
> > +target datalayout =
> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
> > +target triple = "x86_64-unknown-linux-gnu"
>
> > Go ahead and use an -mtriple flag on the llc command and delete these
> lines.
> > If there's no architecture specific need you can try to move it up to the
> > next higher directory.
>
> Right, I was just taking clang -emit-llvm output verbatim and adding
> the CHECK stuff.  I think it is actually target-independent but I have
> no way to verify that.
>
> > +  /// a typedef, a pointer or reference, etc.; or, a data member of a
> > +  /// class/struct/union.
> >
> > The ';' is awkward.
>
> The semicolon separates "data member" from the list of simple derived
> types, which is correct.  Putting a comma there would make "data member"
> just one more element in the list of simple derived types, which it isn't.
>
> I can make the "data member" part be its own sentence but I'd have to
> un-abbreviate "etc." because I was trained not to tolerate abbreviations
> at the end of a sentence (or fragment, in this case).
>
> > +/// getOrCreateContextDIE - Get context owner's DIE.
> > +DIE *CompileUnit::getOrCreateContextDIE(DIDescriptor Context)
> > +{
> >
> > Formatting.
>
> Yep.
>
> > +/// createStaticMemberType - Create debugging information entry for a
> > +/// C++ static data member.
> > +DIType DIBuilder::createStaticMemberType(DIDescriptor Scope, StringRef
> Name,
> > +                                         DIFile File, unsigned
> LineNumber,
> > +                                         DIType Ty, unsigned Flags,
> > +                                         llvm::Value *Val) {
> > +  assert(Flags & DIDescriptor::FlagStaticMember);
> > +  // TAG_member is encoded in DIDerivedType format.
> >
> > If you're going to create a new function for creating static member types
> > why not just or the FlagStaticMember into the existing flags that are
> > passed down rather than making sure it already exists?
>
> Okay, that makes sense. The caller shouldn't have to supply it.
>
> Should have refreshed patches by the end of the day.
> --paulr
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130110/10bbd681/attachment.html>