[codeview] emit debug info for indirect virtual base classes

[inglorion]

Summary

Fixes PR28281.

MSVC lists indirect virtual base classes in the field list of a class.
This change makes Clang emit the information necessary for LLVM to
emit such records.

[rnk]

lib/CodeGen/CGDebugInfo.cpp

1370 ↗

(On Diff #74589)

Make this a DenseSet<const CXXRecordDecl*>, so that we don't need to call getOrCreateType, which is going to do more hash lookups.

1376 ↗

(On Diff #74589)

This should be getCodeGenOpts().EmitCodeView.

1389 ↗

(On Diff #74589)

Some LLVM style idiosyncracies:
http://llvm.org/docs/CodingStandards.html

• Variables start with leading capitals (I know it's weird)
• We don't usually use auto unless the RHS repeats the type in a cast, make_unique, or new.

1392 ↗

(On Diff #74589)

IMO SeenTypes.count(...) would be more idiomatic.

1397 ↗

(On Diff #74589)

You should be able to use this as the key into SeenTypes. You also want to do Base = Base->getCanonicalDecl(), though, so that we don't end up with pointers to different redeclarations of the same record. I don't think I can construct a case where this would matter, but it's probably still worth it. :)

[zturner]

lib/CodeGen/CGDebugInfo.cpp

1389 ↗

(On Diff #74589)

3. Or the type is really gross.
4. Or it's in a range-based for.

1392 ↗

(On Diff #74589)

What a silly function. I wonder why it isn't called contains() and return a bool.

[ruiu]

lib/CodeGen/CGDebugInfo.cpp

1392 ↗

(On Diff #74589)

It's probably because std::set doesn't provide contains() but count() too.

[majnemer]

lib/CodeGen/CGDebugInfo.cpp

1392 ↗

(On Diff #74589)

You can use is_contained from STLExtras.h, it should be maximally idiomatic :)

[inglorion]

@rnk's comments (thanks!)

• Converted SeenTypes to a DenseSet<const CXXRecordDecl*>.
• Switched to getCodeGenOpts().EmitCodeView to check if we should emit the extra records.
• Switched to using SeenTypes.count(...) != 0 to check if we've seen a type.

[inglorion]

• Removed unused header.

[rnk]

Thanks, looks good with some nits

lib/CodeGen/CGDebugInfo.cpp

1393–1395 ↗

(On Diff #74734)

Now that we've figured out the best way to test set membership, I've realized we can save a hash lookup like this:

if (SeenTypes.insert(CanonicalBase).second)
  continue;

1397 ↗

(On Diff #74589)

So, the playing field shifted. Justin Lebar recently added the CanonicalDeclPtr template, which does the getCanonicalDecl dance for you. Probably best to use that. :)

[inglorion]

Updated to track the latest state of D25578

[inglorion]

Use insert's return value to save a set lookup, and use CanonicalDeclPtr

[inglorion]

We also need D25578 in before this can land.

cfe/trunk/lib/CodeGen/CGDebugInfo.h

 // This is the source-level debug info generator for llvm translation.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_CODEGEN_CGDEBUGINFO_H
 #define LLVM_CLANG_LIB_CODEGEN_CGDEBUGINFO_H
 
 #include "CGBuilder.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Allocator.h"
 
 namespace llvm {
 class MDNode;
 }
 
 namespace clang {
-class CXXMethodDecl;
 class ClassTemplateSpecializationDecl;
 class GlobalDecl;
 class ModuleMap;
 class ObjCInterfaceDecl;
 class ObjCIvarDecl;
 class UsingDecl;
 class VarDecl;
 
#include "clang/Basic/OpenCLImageTypes.def"
 
   /// A helper function to collect debug info for C++ base
   /// classes. This is used while creating debug info entry for a
   /// Record.
   void CollectCXXBases(const CXXRecordDecl *Decl, llvm::DIFile *F,
                        SmallVectorImpl<llvm::Metadata *> &EltTys,
                        llvm::DIType *RecordTy);
 
+  /// Helper function for CollectCXXBases.
+  /// Adds debug info entries for types in Bases that are not in SeenTypes.
+  void CollectCXXBasesAux(const CXXRecordDecl *RD, llvm::DIFile *Unit,
+                          SmallVectorImpl<llvm::Metadata *> &EltTys,
+                          llvm::DIType *RecordTy,
+                          const CXXRecordDecl::base_class_const_range &Bases,
+                          llvm::DenseSet<CanonicalDeclPtr<const CXXRecordDecl>> &SeenTypes,
+                          llvm::DINode::DIFlags StartingFlags);
+
   /// A helper function to collect template parameters.
   llvm::DINodeArray CollectTemplateParams(const TemplateParameterList *TPList,
                                           ArrayRef<TemplateArgument> TAList,
                                           llvm::DIFile *Unit);
   /// A helper function to collect debug info for function template
   /// parameters.
   llvm::DINodeArray CollectFunctionTemplateParams(const FunctionDecl *FD,
                                                   llvm::DIFile *Unit);

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

 //===--- CGDebugInfo.cpp - Emit Debug Information for a Module ------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This coordinates the debug information generation while generating code.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CGDebugInfo.h"
 #include "CGBlocks.h"
-#include "CGRecordLayout.h"
 #include "CGCXXABI.h"
 #include "CGObjCRuntime.h"
+#include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/ModuleMap.h"
 #include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
EltTys.push_back(MI == SPCache.end()
                          ? CreateCXXMemberFunction(Method, Unit, RecordTy)
                          : static_cast<llvm::Metadata *>(MI->second));
   }
 }
 
 void CGDebugInfo::CollectCXXBases(const CXXRecordDecl *RD, llvm::DIFile *Unit,
                                   SmallVectorImpl<llvm::Metadata *> &EltTys,
                                   llvm::DIType *RecordTy) {
-  const ASTRecordLayout &RL = CGM.getContext().getASTRecordLayout(RD);
-  for (const auto &BI : RD->bases()) {
-    llvm::DINode::DIFlags BFlags = llvm::DINode::FlagZero;
-    uint64_t BaseOffset;
+  llvm::DenseSet<CanonicalDeclPtr<const CXXRecordDecl>> SeenTypes;
+  CollectCXXBasesAux(RD, Unit, EltTys, RecordTy, RD->bases(), SeenTypes,
+                     llvm::DINode::FlagZero);
 
+  // If we are generating CodeView debug info, we also need to emit records for
+  // indirect virtual base classes.
+  if (CGM.getCodeGenOpts().EmitCodeView) {
+    CollectCXXBasesAux(RD, Unit, EltTys, RecordTy, RD->vbases(), SeenTypes,
+                       llvm::DINode::FlagIndirectVirtualBase);
+  }
+}
+
+void CGDebugInfo::CollectCXXBasesAux(
+    const CXXRecordDecl *RD, llvm::DIFile *Unit,
+    SmallVectorImpl<llvm::Metadata *> &EltTys, llvm::DIType *RecordTy,
+    const CXXRecordDecl::base_class_const_range &Bases,
+    llvm::DenseSet<CanonicalDeclPtr<const CXXRecordDecl>> &SeenTypes,
+    llvm::DINode::DIFlags StartingFlags) {
+  const ASTRecordLayout &RL = CGM.getContext().getASTRecordLayout(RD);
+  for (const auto &BI : Bases) {
     const auto *Base =
         cast<CXXRecordDecl>(BI.getType()->getAs<RecordType>()->getDecl());
+    if (!SeenTypes.insert(Base).second)
+      continue;
+    auto *BaseTy = getOrCreateType(BI.getType(), Unit);
+    llvm::DINode::DIFlags BFlags = StartingFlags;
+    uint64_t BaseOffset;
 
     if (BI.isVirtual()) {
       if (CGM.getTarget().getCXXABI().isItaniumFamily()) {
         // virtual base offset offset is -ve. The code generator emits dwarf
         // expression where it expects +ve number.
         BaseOffset = 0 - CGM.getItaniumVTableContext()
                              .getVirtualBaseOffsetOffset(RD, Base)
                              .getQuantity();
       } else {
         // In the MS ABI, store the vbtable offset, which is analogous to the
         // vbase offset offset in Itanium.
         BaseOffset =
             4 * CGM.getMicrosoftVTableContext().getVBTableIndex(RD, Base);
       }
-      BFlags = llvm::DINode::FlagVirtual;
+      BFlags |= llvm::DINode::FlagVirtual;
     } else
       BaseOffset = CGM.getContext().toBits(RL.getBaseClassOffset(Base));
     // FIXME: Inconsistent units for BaseOffset. It is in bytes when
     // BI->isVirtual() and bits when not.
 
     BFlags |= getAccessFlag(BI.getAccessSpecifier(), RD);
-    llvm::DIType *DTy = DBuilder.createInheritance(
-        RecordTy, getOrCreateType(BI.getType(), Unit), BaseOffset, BFlags);
+    llvm::DIType *DTy =
+        DBuilder.createInheritance(RecordTy, BaseTy, BaseOffset, BFlags);
     EltTys.push_back(DTy);
   }
 }
 
 llvm::DINodeArray
 CGDebugInfo::CollectTemplateParams(const TemplateParameterList *TPList,
                                    ArrayRef<TemplateArgument> TAList,
                                    llvm::DIFile *Unit) {

cfe/trunk/test/CodeGenCXX/debug-info-ms-vbase.cpp

 
 // CHECK: ![[SecondaryVTable_base]] = !DIDerivedType(tag: DW_TAG_inheritance, scope: ![[HasPrimaryBase]],
 // CHECK-SAME: baseType: ![[SecondaryVTable:[0-9]+]], offset: 4, flags: DIFlagVirtual)
 
 // CHECK: ![[SecondaryVTable]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondaryVTable"
 
 // CHECK: ![[HasVirtualMethod_base]] = !DIDerivedType(tag: DW_TAG_inheritance, scope: ![[HasPrimaryBase]], baseType: ![[HasVirtualMethod]])
 
+// CHECK: ![[HasIndirectVirtualBase:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "HasIndirectVirtualBase"
+// CHECK-SAME: elements: ![[elements:[0-9]+]]
+
+// CHECK: !DIDerivedType(tag: DW_TAG_inheritance, scope: ![[HasIndirectVirtualBase]], baseType: ![[HasPrimaryBase]]
+// CHECK-NOT: DIFlagIndirectVirtualBase
+// CHECK-SAME: )
+
+// CHECK: !DIDerivedType(tag: DW_TAG_inheritance, scope: ![[HasIndirectVirtualBase]], baseType: ![[SecondaryVTable]]
+// CHECK-SAME: flags:
+// CHECK-SAME: DIFlagIndirectVirtualBase
+
 // CHECK: ![[DynamicNoVFPtr:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "DynamicNoVFPtr",
 // CHECK-SAME: elements: ![[elements:[0-9]+]]
 
 // CHECK: ![[elements]] = !{![[POD_base:[0-9]+]]}
 
 // CHECK: ![[POD_base]] = !DIDerivedType(tag: DW_TAG_inheritance, scope: ![[DynamicNoVFPtr]],
 // CHECK-SAME: baseType: ![[POD:[0-9]+]], offset: 4, flags: DIFlagVirtual)
 

 NoPrimaryBase no_primary_base;
 
 struct SecondaryVTable { virtual void g(); };
 
 struct HasPrimaryBase : virtual SecondaryVTable, HasVirtualMethod { };
 
 HasPrimaryBase has_primary_base;
 
+struct HasIndirectVirtualBase : public HasPrimaryBase {};
+
+HasIndirectVirtualBase has_indirect_virtual_base;