[DebugInfo][Clang] record the access flag for class/struct/union types.

[Esme]

Summary

This patch emits DW_AT_accessibility attribute for class/struct/union types.

Sample program :

class A {
  public:
    struct B {
    };
  public:
    B b;
};

struct C {
  private:
    union D {
    };
  public:
    D d;
};

union E {
  private:
    class F {
    };
    class G {
    };
  public:
    F f;
    G g;
};

Debug info without this patch:

0x0000013d:   DW_TAG_class_type
                DW_AT_name      ("A")

0x00000143:     DW_TAG_member
                  DW_AT_name    ("b")
                  DW_AT_accessibility   (DW_ACCESS_public)

0x0000014e:     DW_TAG_structure_type
                  DW_AT_name    ("B") // no accessibility attribute is present, private access is assumed for B.

0x00000155:   DW_TAG_structure_type
                DW_AT_name      ("C")

0x0000015b:     DW_TAG_member
                  DW_AT_name    ("d")

0x00000165:     DW_TAG_union_type
                  DW_AT_name    ("D") // no accessibility attribute is present, public access is assumed for D.

0x0000016c:   DW_TAG_union_type
                DW_AT_name      ("E")

0x00000172:     DW_TAG_member
                  DW_AT_name    ("f")

0x0000017c:     DW_TAG_class_type
                  DW_AT_name    ("F") // no accessibility attribute is present, public access is assumed for F.

0x00000182:     DW_TAG_member
                  DW_AT_name    ("g")

0x0000018c:     DW_TAG_class_type
                  DW_AT_name    ("G") // no accessibility attribute is present, public access is assumed for G.

[dblaikie]

Generally sounds OK to me - this should be submitted as two separate patches. The LLVM part first, followed by the Clang part (since they can be separated they should be, and I guess the order doesn't matter too much (since the new IR that Clang's generating won't be invalid/break LLVM, it'll just be unused until the LLVM part is submitted).

I probably wouldn't bother adding the helper functions for "getParentDecl" to DeclCXX.h since there's only one use - if they provide a bunch of cleanup to existing callers, then maybe - otherwise just inlining the logic into the one call site is probably the right thing to do at the moment.

[Esme]

Thanks! @dblaikie
Separate into Clang part (this patch) and LLVM part (D115606).

[shchenz]

clang/test/CodeGenCXX/debug-info-access.cpp

28

Will we generate a redundant flag if we define a private type in the class or a public type in the struct/union? Maybe we could add a case for that too.

[dblaikie]

clang/test/CodeGenCXX/debug-info-access.cpp

28

How',s this compare to member variables? Do we always put the access level on members even when it's the default? Perhaps the Flag handling code could be generalized and used between both these cases & if one of them gets fixed in favor of not explicitly specifying the default, both would benefit.

[Esme]

Add a case to show that the default access flag is not recorded.

clang/test/CodeGenCXX/debug-info-access.cpp

28

Member variables use the same Flag handling code, i.e. getAccessFlag(), which returns 0 if the access specifier equals the default for the containing type.
Therefore for both cases we will not record the access flag when it's the default.
The case where the default access is not explicitly specified is added now.

[shchenz]

clang/test/CodeGenCXX/debug-info-access.cpp

28

Good to know getAccessFlag() already takes care of this case. Thanks

[shchenz]

Thanks for fixing this. LGTM. Please wait for @dblaikie comments.

[dblaikie]

Got any data on how much this (combined with the LLVM patch) increases debug info size of, say, a clang self-host build? I assume not much, but wouldn't hurt to know.

(does GCC produce this sort of debug info, or does it skip the access specifiers on nested types?)

[Esme]

In D115503#3188302, @dblaikie wrote:

Got any data on how much this (combined with the LLVM patch) increases debug info size of, say, a clang self-host build? I assume not much, but wouldn't hurt to know.

section	before	after
.debug_loc	929821	929821
.debug_abbrev	5885289	5971547
.debug_info	497613455	498122074
.debug_ranges	45731664	45731664
.debug_str	233842595	233839388
.debug_line	149773166	149764583

I built two self-host Clangs with and without the two patches and got the results shown in the table, and I think the increase of size is acceptable?

(does GCC produce this sort of debug info, or does it skip the access specifiers on nested types?)

Yes, GCC also emits the access attribute for nested types when the access level doesn't equal to the default.

[dblaikie]

In D115503#3191156, @Esme wrote:

In D115503#3188302, @dblaikie wrote:

Got any data on how much this (combined with the LLVM patch) increases debug info size of, say, a clang self-host build? I assume not much, but wouldn't hurt to know.

section	before	after
.debug_loc	929821	929821
.debug_abbrev	5885289	5971547
.debug_info	497613455	498122074
.debug_ranges	45731664	45731664
.debug_str	233842595	233839388
.debug_line	149773166	149764583

I built two self-host Clangs with and without the two patches and got the results shown in the table, and I think the increase of size is acceptable?

(does GCC produce this sort of debug info, or does it skip the access specifiers on nested types?)

Yes, GCC also emits the access attribute for nested types when the access level doesn't equal to the default.

Thanks for the data - looks good to me. Maybe include some of that data (summary of total binary size change/total debug info size change - and if you could include the flags (was this an -O0 -g build? Optimized (at what level)? Compressed debug info (-gz)? etc) that'd be helpful to better understand the comparison.

[Esme]

In D115503#3192840, @dblaikie wrote:

Thanks for the data - looks good to me. Maybe include some of that data (summary of total binary size change/total debug info size change - and if you could include the flags (was this an -O0 -g build? Optimized (at what level)? Compressed debug info (-gz)? etc) that'd be helpful to better understand the comparison.

They are built with flags of clang -O0 -g (no -gz) and the table with summary changes is updated as follow.

section	before	after	change	%
.debug_loc	929821	929821	0	0
.debug_abbrev	5885289	5971547	+86258	+1.466%
.debug_info	497613455	498122074	+508619	+0.102%
.debug_ranges	45731664	45731664	0	0
.debug_str	233842595	233839388	-3207	-0.001%
.debug_line	149773166	149764583	-8583	-0.006%
total (debug)	933775990	934359077	+583087	+0.062%

total (binary)

1394617288

1395200024

+582736

+0.042%

[dblaikie]

In D115503#3193978, @Esme wrote:

In D115503#3192840, @dblaikie wrote:

Thanks for the data - looks good to me. Maybe include some of that data (summary of total binary size change/total debug info size change - and if you could include the flags (was this an -O0 -g build? Optimized (at what level)? Compressed debug info (-gz)? etc) that'd be helpful to better understand the comparison.

They are built with flags of clang -O0 -g (no -gz) and the table with summary changes is updated as follow.

section	before	after	change
.debug_loc	929821	929821	0
.debug_abbrev	5885289	5971547	+86258
.debug_info	497613455	498122074	+508619
.debug_ranges	45731664	45731664	0
.debug_str	233842595	233839388	-3207
.debug_line	149773166	149764583	-8583
total (debug)	933775990	934359077	+583087

total (binary)	1394617288	1395200024	+582736

Ah, cool - could you include % growth on those rows

(hmm, .debug_line and .debug_str shouldn't be changing in size with this change, right? If you use the same clang version to test the two cases (if you're using a bootstrap with/without the patch applied, then the patch changes itself would show up as changes here))

& if you could include this table in the commit message, that'd be great!

(can you commit this yourself, or do you need someone to do that for you?)

[Esme]

In D115503#3195171, @dblaikie wrote:

Ah, cool - could you include % growth on those rows

Thanks, I edited the table in the previous comment.

(hmm, .debug_line and .debug_str shouldn't be changing in size with this change, right? If you use the same clang version to test the two cases (if you're using a bootstrap with/without the patch applied, then the patch changes itself would show up as changes here))

Hmm...it's a little strange. I got these data by:

1. pull a base llvm-project branch --> source codes
2. build the source --> clang1
3. build the codes after applying the patch --> clang2
4. use clang1 to build the source (no patch) --> clang-before
5. use clang2 to build the source (no patch) --> clang-after
6. compare the size between clang-before and clang-after

I reproduced these steps on another base branch, and saw similar changes (i.e. .debug_line and .debug_str changed in size)...
But I think the change is so small as to be negligible?

& if you could include this table in the commit message, that'd be great!

(can you commit this yourself, or do you need someone to do that for you?)

I will include this table when committing, thanks!

[shchenz]

In D115503#3199459, @Esme wrote:

In D115503#3195171, @dblaikie wrote:

Ah, cool - could you include % growth on those rows

Thanks, I edited the table in the previous comment.

(hmm, .debug_line and .debug_str shouldn't be changing in size with this change, right? If you use the same clang version to test the two cases (if you're using a bootstrap with/without the patch applied, then the patch changes itself would show up as changes here))

Hmm...it's a little strange. I got these data by:

1. pull a base llvm-project branch --> source codes
2. build the source --> clang1
3. build the codes after applying the patch --> clang2
4. use clang1 to build the source (no patch) --> clang-before
5. use clang2 to build the source (no patch) --> clang-after
6. compare the size between clang-before and clang-after

I reproduced these steps on another base branch, and saw similar changes (i.e. .debug_line and .debug_str changed in size)...
But I think the change is so small as to be negligible?

& if you could include this table in the commit message, that'd be great!

(can you commit this yourself, or do you need someone to do that for you?)

I will include this table when committing, thanks!

.debug_str will be changed because of DW_AT_producer? We are using different compilers to compile the source.

[dblaikie]

In D115503#3199489, @shchenz wrote:

In D115503#3199459, @Esme wrote:

In D115503#3195171, @dblaikie wrote:

Ah, cool - could you include % growth on those rows

Thanks, I edited the table in the previous comment.

(hmm, .debug_line and .debug_str shouldn't be changing in size with this change, right? If you use the same clang version to test the two cases (if you're using a bootstrap with/without the patch applied, then the patch changes itself would show up as changes here))

Hmm...it's a little strange. I got these data by:

1. pull a base llvm-project branch --> source codes
2. build the source --> clang1
3. build the codes after applying the patch --> clang2
4. use clang1 to build the source (no patch) --> clang-before
5. use clang2 to build the source (no patch) --> clang-after
6. compare the size between clang-before and clang-after

I reproduced these steps on another base branch, and saw similar changes (i.e. .debug_line and .debug_str changed in size)...
But I think the change is so small as to be negligible?

& if you could include this table in the commit message, that'd be great!

(can you commit this yourself, or do you need someone to do that for you?)

I will include this table when committing, thanks!

.debug_str will be changed because of DW_AT_producer? We are using different compilers to compile the source.

ah, fair enough

clang/lib/CodeGen/CGDebugInfo.cpp

if (auto CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
 
     // Record if a C++ record is non-trivial type.
     if (!CXXRD->isTrivial())
       Flags |= llvm::DINode::FlagNonTrivial;
 
     // Record exports it symbols to the containing structure.
     if (CXXRD->isAnonymousStructOrUnion())
         Flags |= llvm::DINode::FlagExportSymbols;
+
+    Flags |= getAccessFlag(CXXRD->getAccess(),
+                           dyn_cast<CXXRecordDecl>(CXXRD->getDeclContext()));
   }
 
   llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(D);
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
       getTagForRecord(RD), RDName, RDContext, DefUnit, Line, 0, Size, Align,
       Flags, Identifier, Annotations);
 
   // Elements of composite types usually have back to the type, creating

clang/test/CodeGenCXX/debug-info-access.cpp

 protected:
   // CHECK: !DISubprogram(name: "prot",{{.*}} line: [[@LINE+1]],{{.*}} flags: DIFlagProtected | DIFlagPrototyped,
   void prot();
 private:
   // CHECK: !DISubprogram(name: "priv_default",{{.*}} line: [[@LINE+1]],{{.*}} flags: DIFlagPrototyped,
   void priv_default();
 };
 
+class C {
+public:
+  // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "D",{{.*}} flags: DIFlagPublic | DIFlagTypePassByValue,
+  struct D {
+  };
+protected:
+  // CHECK: !DICompositeType(tag: DW_TAG_union_type, name: "E",{{.*}} flags: DIFlagProtected | DIFlagTypePassByValue,
+  union E {
+  };
+private:
+  // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "J",{{.*}} flags: DIFlagTypePassByValue,
+  struct J {
+  };
+public:
+  D d;
+  E e;
+  J j;
+};
+
+struct F {
+private:
+  // CHECK: !DICompositeType(tag: DW_TAG_union_type, name: "G",{{.*}} flags: DIFlagPrivate | DIFlagTypePassByValue,
+  union G {
+  };
+public:
+  G g;
+};
+
+union H {
+private:
+  // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "I",{{.*}} flags: DIFlagPrivate | DIFlagTypePassByValue,
+  class I {
+  };
+public:
+  I i;
+};
+
 union U {
   // CHECK-DAG: !DISubprogram(name: "union_pub_default",{{.*}} line: [[@LINE+1]],{{.*}} flags: DIFlagPrototyped,
   void union_pub_default();
 private:
   // CHECK-DAG: !DIDerivedType(tag: DW_TAG_member, name: "union_priv",{{.*}} line: [[@LINE+1]],{{.*}} flags: DIFlagPrivate)
   int union_priv;
 };
 
-
 // CHECK: !DISubprogram(name: "free",
 // CHECK-SAME:          flags: DIFlagPrototyped,
 // CHECK-SAME:          spFlags: DISPFlagDefinition
 void free() {}
 
 U u;
 A a;
 B b;
+C c;
+F f;
+H h;