[DebugInfo] Add a new DI flag to record if a C++ record is a trivial type

[asmith]

Summary

This flag is used when emitting debug info and is 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.

It is not easy to determine if a C++ record is trivial or not based on the current DICompositeType flags and other accessible debug information from Codeview. For example, without this flag the metadata for a non-trivial C++ record with user-defined ctor and a trivial one with a defaulted ctor are the same.

struct S { S(); }
struct S { S() = default; }

This change introduces a new DI flag and corresponding clang::CXXRecordDecl::isTrivial method to set the flag in the frontend.

[probinson]

Is "trivial" the right flag? In DWARF v5 there is a "defaulted" attribute, which also distinguishes between in-class and out-of-class defaulting. I'd rather not have a bunch of different flags meaning only slightly different things if we can come up with a more coherent way to track the information we need.

llvm/IR/DebugInfoFlags.def

49 ↗

(On Diff #140540)

Is there some reason FixedEnum wants to be last?

[aprantl]

Have you looked at how the new trivial_abi attribute is handled in DWARF? We set the calling convention of C++ types accordingly. Could you just reuse the same information in CodeView?

[aprantl]

See test/CodeGenCXX/debug-info-composite-cc.cpp for reference.

[dblaikie]

Yeah, +1 for verifying whether or not DIFlagTypePassByValue/Reference is
sufficient for this.

[asmith]

I don't think those DWARF flags help in this case but let's check. Here are the results of changing the flag as suggested and rerunning the test from D45123. We can see that several places that are not marked as Trivial are marked as PassByValue meaning that we cannot rely on PassByValue to mean Trivial.

$ grep ByValue function-options.ll

// These are not Trivial but they are PassByValue

!19 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "BClass", file: !9, line: 10, size: 8, flags: DIFlagTypePassByValue, elements: !20, identifier: ".?AVBClass@@")
!43 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "C2Class", file: !9, line: 25, size: 8, flags: DIFlagTypePassByValue, elements: !44, identifier: ".?AVC2Class@@")
!55 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "DClass", file: !9, line: 32, size: 8, flags: DIFlagTypePassByValue, elements: !56, identifier: ".?AVDClass@@")
!107 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "BStruct", file: !9, line: 63, size: 8, flags: DIFlagTypePassByValue, elements: !108, identifier: ".?AUBStruct@@")

If you look at the corresponding change to clang in D45124 you will see that Trivial and PassByValue/Reference are a bit different.

if (auto CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
  if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
    Flags |= llvm::DINode::FlagTypePassByReference;
  else
    Flags |= llvm::DINode::FlagTypePassByValue;

  // When emitting codeview, record if a C++ record is trivial type.
  if (CGM.getCodeGenOpts().EmitCodeView) {
    if (CXXRD->isTrivial())
      Flags |= llvm::DINode::FlagTrivial;
  }
}

[dblaikie]

Does the proposed clang change (using 'isTrivial') match MSVC's behavior in
these cases?

Any idea what this information is used for by the debugger? (because it
seems /likely/ it uses it for similar things to the PassByValue/Reference
stuff - that's something important to a debugger)

[asmith]

The meaning of trivial is the same. There are some differences with MSVC in the generated CodeView that are documented in the FIXMEs in D45123.

LLDB is going to use this to construct a CXXRecordDecl when parsing record symbols (PDBSymbolTypeUdt and PDBSymbolBaseclass). FunctionOptions, ClassOptions and MethodOptions rely on this information which is currently missing in LLVMs CV.

See D44406 for what kicked all this off originally.

[dblaikie]

I'm still not sure this description/name matches the examples given in the
other review from Reid.

Sounds like this is more about whether it has a trivial default
constructor, specifically? (which, yes, is different from the triviality of
copy construction which is relevant to the DWARF flag we already have) -
some test cases/demonstrations of the MSVC behavior might be helpful?

[asmith]

D44406 is not completely the same as this one yes. Did you look at the
MSVC examples in D45123?

[dblaikie]

This still doesn't /sound/ right to me - the triviality of a class doesn't
seem like it would correctly allow you to deduce the triviality of specific
special members? Because the the class would only be considered trivial if
all the special members are trivial, probably?

I suspect the CxxReturnUdt might be related to the PassByValue/Reference
feature we already have for DWARF.

For example does MSVC put CxxReturnUdt on return types where they have a
non-trivial default ctor, but a trivial copy ctor? or other variations like
that.

Reid/Zach - you two looked at this sort of thing & have a better sense of
what each of these flags represent? (maybe ideas about what we should do
for types marked with Clang's [[trivial_abi]] attribute (obviously wouldn't
be interoperable with code compiled with MSVC since it doesn't support the
attribute - but still be good to know how to describe the type to the
debugger)?

[rnk]

In D44406, I was imagining instead that we'd mark DISubprograms as trivial or non-trivial, not the type itself, and then CodeViewDebug.cpp in LLVM could skip trivial constructors when looking for classes.

I think for CxxReturnUdt we can go ahead and use the DIFlagPassByReference marker. The flag name isn't really accurate since it also covers cases that would use inalloca. My ideas aren't that much better, though: DIFlagPassingIsNonTrivial, DIFlagCopyIsNonTrivial, DIFlagNonTriviallyCopyable, blech.

[asmith]

Only considering DIFlagPassByReference for CxxReturnUdt will not cover all
cases. There are times when DIFlagPassByValue is also CxxReturnUdt.

[rnk]

In D45122#1063889, @asmith wrote:

Only considering DIFlagPassByReference for CxxReturnUdt will not cover all
cases. There are times when DIFlagPassByValue is also CxxReturnUdt.

Can you describe them? Sorry, I can't investigate myself right now because my Windows workstation died over the weekend.

I'm guessing the issue is in this code:

if (auto CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
  if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
    Flags |= llvm::DINode::FlagTypePassByReference;
  else
    Flags |= llvm::DINode::FlagTypePassByValue;
}

getRecordArgABI returns three possible results, and this code only effectively handles two. I think it would be correct to:

1. Rename FlagTypePassByReference to FlagPassNonTrivial (or something like that but less terrible)
2. Set the flag when the RAA is either indirect or RAA_DirectInMemory.

Truly, the minimal change is to invert the logic of the if to check for RAA_Default and set FlagTypePassByReference in all other cases. That's how LLVM sees inalloca anyway. =p

[asmith]

Please read the previous discussion in this code review for the examples of why those DWARF flags don't work.

"I don't think those DWARF flags help in this case but let's check. Here are the results of changing the flag as suggested..."

[zturner]

I'll ask Reid to take another look at this next week. Sorry for the delay, he's been on a long vacation.

[rnk]

lgtm

We should also be able to use this to mark explicitly defaulted but trivial special members, i.e.:

struct Foo {
  int x;
  ~Foo() = default;
};
Foo gv;

Right now we emit a destructor, which is silly.

llvm/trunk/include/llvm-c/DebugInfo.h

typedef enum {
   LLVMDIFlagMultipleInheritance = 2 << 16,
   LLVMDIFlagVirtualInheritance = 3 << 16,
   LLVMDIFlagIntroducedVirtual = 1 << 18,
   LLVMDIFlagBitField = 1 << 19,
   LLVMDIFlagNoReturn = 1 << 20,
   LLVMDIFlagMainSubprogram = 1 << 21,
   LLVMDIFlagTypePassByValue = 1 << 22,
   LLVMDIFlagTypePassByReference = 1 << 23,
+  LLVMDIFlagFixedEnum = 1 << 24,
+  LLVMDIFlagThunk = 1 << 25,
+  LLVMDIFlagTrivial = 1 << 26,
   LLVMDIFlagIndirectVirtualBase = (1 << 2) | (1 << 5),
   LLVMDIFlagAccessibility = LLVMDIFlagPrivate | LLVMDIFlagProtected |
                             LLVMDIFlagPublic,
   LLVMDIFlagPtrToMemberRep = LLVMDIFlagSingleInheritance |
                              LLVMDIFlagMultipleInheritance |
                              LLVMDIFlagVirtualInheritance
 } LLVMDIFlags;
 

llvm/trunk/include/llvm/IR/DebugInfoFlags.def

 HANDLE_DI_FLAG((1 << 18), IntroducedVirtual)
 HANDLE_DI_FLAG((1 << 19), BitField)
 HANDLE_DI_FLAG((1 << 20), NoReturn)
 HANDLE_DI_FLAG((1 << 21), MainSubprogram)
 HANDLE_DI_FLAG((1 << 22), TypePassByValue)
 HANDLE_DI_FLAG((1 << 23), TypePassByReference)
 HANDLE_DI_FLAG((1 << 24), FixedEnum)
 HANDLE_DI_FLAG((1 << 25), Thunk)
+HANDLE_DI_FLAG((1 << 26), Trivial)
 
 // To avoid needing a dedicated value for IndirectVirtualBase, we use
 // the bitwise or of Virtual and FwdDecl, which does not otherwise
 // make sense for inheritance.
 HANDLE_DI_FLAG((1 << 2) | (1 << 5), IndirectVirtualBase)
 
 #ifdef DI_FLAG_LARGEST_NEEDED
 // intended to be used with ADT/BitmaskEnum.h
 // NOTE: always must be equal to largest flag, check this when adding new flag
-HANDLE_DI_FLAG((1 << 25), Largest)
+HANDLE_DI_FLAG((1 << 26), Largest)
 #undef DI_FLAG_LARGEST_NEEDED
 #endif
 
 #undef HANDLE_DI_FLAG