[DebugInfo] Combine Trivial and NonTrivial flags

[asmith]

Summary

These flags are used when emitting debug info and needed to initialize subprogram and member function attributes (function options) for Codeview. These function options are used to create an accurate compiler type for UDT symbols (class/struct/union) from PDBs.

The Trivial flag was introduced in https://reviews.llvm.org/D45122

It's been pointed out that Trivial and NonTrivial may imply each other and that seems to be the case in the current tests. This change combines them into a single flag -- NonTrivial -- and updates the corresponding unit tests. There is an additional change to llvm to update the flags.

[probinson]

I'd preserve the trivial test cases and show that the NonTrivial flag is *not* present. I can suggest ways to achieve this if you like (note there is no CHECK-DAG-NOT combined directive).

test/CodeGenCXX/debug-info-composite-triviality.cpp

88 ↗

(On Diff #190560)

This one used to be flagged nontrivial, and now it's not flagged?

[dblaikie]

Would it be simpler/better to revert all the FlagTrivial work? & use the FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been in-tree longer)

[asmith]

Either way. It was fewer changes to use FlagNonTrivial. Which flag do people want to keep?

[probinson]

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

[dblaikie]

@asmith: Where's the LLVM-side change/review that goes with this, btw?

In D59347#1442970, @probinson wrote:

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

Fair enough - I was mostly coming at this from the "the patch that was committed should be reverted" & then we could haggle over other things, but fair point.

[asmith]

Okay if we change the flag then I believe the tests under llvm/tests/lib/DebugInfo/CodeView must be updated. If we use NonTrivial all the existing tests work as expected.

LLVM change is here (you are all reviewers).
https://reviews.llvm.org/D59348

[dblaikie]

In D59347#1443051, @dblaikie wrote:

@asmith: Where's the LLVM-side change/review that goes with this, btw?

In D59347#1442970, @probinson wrote:

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

Fair enough - I was mostly coming at this from the "the patch that was committed should be reverted" & then we could haggle over other things, but fair point.

Hmm, one other thought: Technically "non trivial" is perhaps more accurate/less error prone. Only marking structures as "trivial" but other types without that marker makes it more subtle (since not all trivial types would be marked trivial - only those of a classification that means they /could/ be non-trivial). Whereas marking the non-trivial types is more broadly accurate.

@asmith - is this patch now essentially a revert of the trivial flag addition? Or are there any parts that were not reverted, if so, why not? (I would expect/imagine all the testing could be reverted too - since the NonTrivial flag was presumably already tested appropriately?)

[probinson]

In D59347#1444819, @dblaikie wrote:

In D59347#1443051, @dblaikie wrote:

@asmith: Where's the LLVM-side change/review that goes with this, btw?

In D59347#1442970, @probinson wrote:

As a rule I would prefer flags with positive names, as it's slightly easier to read !isTrivial than !isNonTrivial. And trivially shorter. :-)

Fair enough - I was mostly coming at this from the "the patch that was committed should be reverted" & then we could haggle over other things, but fair point.

Hmm, one other thought: Technically "non trivial" is perhaps more accurate/less error prone. Only marking structures as "trivial" but other types without that marker makes it more subtle (since not all trivial types would be marked trivial - only those of a classification that means they /could/ be non-trivial). Whereas marking the non-trivial types is more broadly accurate.

Well, that's a reasonable point in favor of keeping the NonTrivial flag.

[Hui]

test/CodeGenCXX/debug-info-composite-triviality.cpp

88 ↗

(On Diff #190560)

The base struct 'Trivial' was removed and this case became duplicated since it is now similar with 'struct NonTrivialD'. Could be very easy to add it back per request.

[probinson]

test/CodeGenCXX/debug-info-composite-triviality.cpp

88 ↗

(On Diff #190560)

I think it's worth having a multiple-inheritance case.

[asmith]

This should be ready to commit

[dblaikie]

Approved pending the LLVM side changes/discussion.

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
   // Explicitly record the calling convention for C++ records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
     if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
       Flags |= llvm::DINode::FlagTypePassByReference;
     else
       Flags |= llvm::DINode::FlagTypePassByValue;
 
-    // Record if a C++ record is trivial type.
-    if (CXXRD->isTrivial())
-      Flags |= llvm::DINode::FlagTrivial;
-    else
+    // Record if a C++ record is non-trivial type.
+    if (!CXXRD->isTrivial())
       Flags |= llvm::DINode::FlagNonTrivial;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
       getTagForRecord(RD), RDName, RDContext, DefUnit, Line, 0, Size, Align,
       Flags, Identifier);
 
   // Elements of composite types usually have back to the type, creating

cfe/trunk/test/CodeGenCXX/debug-info-composite-triviality.cpp

 
 
 // Check a non-composite type
 // CHECK-DAG: !DIGlobalVariable(name: "GlobalVar", {{.*}}type: {{.*}}, isLocal: false, isDefinition: true)
 int GlobalVar = 0;
 
 // Cases to test composite type's triviality
 
-// CHECK-DAG: !DICompositeType({{.*}}, name: "Union",{{.*}}flags: {{.*}}DIFlagTrivial
+// CHECK-DAG:      {{.*}}!DIGlobalVariable(name: "Union",
+// CHECK-DAG-NEXT: {{^((?!\bDIFlagNonTrivial\b).)*$}}
 union Union {
   int a;
 } Union;
 
-// CHECK-DAG: !DICompositeType({{.*}}, name: "Trivial",{{.*}}flags: {{.*}}DIFlagTrivial
+// CHECK-DAG:      {{.*}}!DIGlobalVariable(name: "Trivial",
+// CHECK-DAG-NEXT: {{^((?!\bDIFlagNonTrivial\b).)*$}}
 struct Trivial {
   int i;
 } Trivial;
 
-// CHECK-DAG: !DICompositeType({{.*}}, name: "TrivialA",{{.*}}flags: {{.*}}DIFlagTrivial
+// CHECK-DAG:       {{.*}}!DIGlobalVariable(name: "TrivialA",
+// CHECK-DAG-NEXT:  {{^((?!\bDIFlagNonTrivial\b).)*$}}
 struct TrivialA {
   TrivialA() = default;
 } TrivialA;
 
-// CHECK-DAG: !DICompositeType({{.*}}, name: "TrivialB",{{.*}}flags: {{.*}}DIFlagTrivial
+// CHECK-DAG:       {{.*}}!DIGlobalVariable(name: "TrivialB",
+// CHECK-DAG-NEXT:  {{^((?!\bDIFlagNonTrivial\b).)*$}}
 struct TrivialB {
   int m;
   TrivialB(int x) { m = x; }
   TrivialB() = default;
 } TrivialB;
 
-// CHECK-DAG: !DICompositeType({{.*}}, name: "TrivialC",{{.*}}flags: {{.*}}DIFlagTrivial
+// CHECK-DAG:       {{.*}}!DIGlobalVariable(name: "TrivialC",
+// CHECK-DAG-NEXT:  {{^((?!\bDIFlagNonTrivial\b).)*$}}
 struct TrivialC {
   struct Trivial x;
 } TrivialC;
 
-// CHECK-DAG: !DICompositeType({{.*}}, name: "TrivialD",{{.*}}flags: {{.*}}DIFlagTrivial
+// CHECK-DAG:       {{.*}}!DIGlobalVariable(name: "TrivialD",
+// CHECK-DAG-NEXT:  {{^((?!\bDIFlagNonTrivial\b).)*$}}
 struct NT {
   NT() {};
 };
 struct TrivialD {
   static struct NT x; // Member is non-trivial but is static.
 } TrivialD;