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

[Robinson, Paul]

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 --------------
A non-text attachment was scrubbed...
Name: Static-member-llvm.diff
Type: application/octet-stream
Size: 25091 bytes
Desc: Static-member-llvm.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130110/52a0f5ba/attachment.obj>