Mark static member functions as static in CodeViewDebug

[amccarth]

Summary

To improve CodeView quality for static member functions, we need to make the
static explicit. In addition to a small change in LLVM's CodeViewDebug to
return the appropriate MethodKind, this requires a small change in Clang to
note the staticness in the debug info metadata.

[rnk]

This should have an IRGen test. You can probably extend clang/test/CodeGenCXX/debug-info-ms-abi.cpp with a static method.

clang/lib/CodeGen/CGDebugInfo.cpp

2907 ↗

(On Diff #114717)

Please revert the unrelated whitespace change

3218 ↗

(On Diff #114717)

ditto

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

327–329 ↗

(On Diff #114717)

I suppose this is where we need to pass down a bool about this being a static member function.

1560–1564 ↗

(On Diff #114717)

If we had a bool IsInstanceMethod or something here we could skip this drop_front() dance and that would fix our parameters.

1625 ↗

(On Diff #114717)

I'd do this check first, since static methods can't be virtual.

[amccarth]

Good tip on the lowering. Extra test coming momentarily.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1560–1564 ↗

(On Diff #114717)

Ah yes, this fixes the Type field for static methods as well. I'll extend the test to show that.

1625 ↗

(On Diff #114717)

And virtual methods can't be static, so I'm not sure the order really matters. But done because it's not worth a debate.

[amccarth]

Adds code-gen test. Corrects problem with first param of static method being
treated as the type of the this pointer. Addresses style suggestions.

[rnk]

Looks good! Make sure to run check-clang and check-llvm. The CGDebugInfo.cpp change might affect clang codegen tests that generate debug info for DWARF.

[amccarth]

In D37715#868973, @rnk wrote:

Looks good! Make sure to run check-clang and check-llvm. The CGDebugInfo.cpp change might affect clang codegen tests that generate debug info for DWARF.

So check-llvm is fine (except for one unrelated test that fails without my change). I've also used all the various combinations of check-*debuginfo*.

check-clang, however, gets hundreds of Python stack traces because:

TypeError: a bytes-like object is required, not 'str'

This sounded to me like a Python 2/3 problem, so I reset my environment to Python 2.7, re-ran CMake, and tried again. But I got the same set of Python errors. What's the trick here?

[rnk]

In D37715#869699, @amccarth wrote:

This sounded to me like a Python 2/3 problem, so I reset my environment to Python 2.7, re-ran CMake, and tried again. But I got the same set of Python errors. What's the trick here?

CMake discovers the python executable and caches it in CMakeCache.txt on first configuration, so changing the environment generally doesn't have an effect unless you re-configure with a clean CMakeCache.txt and CMakeFiles dir.

The quick way to get back to 2.7 is probably to edit CMakeCache.txt and change the PYTHON_EXECUTABLE variable to look something like this:

PYTHON_EXECUTABLE:FILEPATH=C:/Python27/python.exe

ninja check-clang will pick that up on the next run and use it to run the test suite.

Immediately issue aside, I guess it's worth setting up continuous testing with py3k. =/

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

if (CGM.getTarget().getCXXABI().isItaniumFamily()) {
       // it knows the dynamic type of an object.
       ThisAdjustment = CGM.getCXXABI()
                            .getVirtualFunctionPrologueThisAdjustment(GD)
                            .getQuantity();
     }
     ContainingType = RecordTy;
   }
 
+  if (Method->isStatic())
+    Flags |= llvm::DINode::FlagStaticMember;
   if (Method->isImplicit())
     Flags |= llvm::DINode::FlagArtificial;
   Flags |= getAccessFlag(Method->getAccess(), Method->getParent());
   if (const auto *CXXC = dyn_cast<CXXConstructorDecl>(Method)) {
     if (CXXC->isExplicit())
       Flags |= llvm::DINode::FlagExplicit;
   } else if (const auto *CXXC = dyn_cast<CXXConversionDecl>(Method)) {
     if (CXXC->isExplicit())

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

 // RUN: %clang_cc1 %s -triple=i686-pc-windows-msvc -debug-info-kind=limited -gcodeview -emit-llvm -o - | FileCheck %s
 
 // Tests that certain miscellaneous features work in the MS ABI.
 
 struct Foo {
   virtual void f();
   virtual void g();
   virtual void h();
+  static void i(int, int);
   struct Nested {};
 };
 Foo f;
 Foo::Nested n;
 
 // CHECK: ![[Nested:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Nested",
 // CHECK-SAME: identifier: ".?AUNested@Foo@@"
 
 // CHECK: ![[Foo:[^ ]*]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Foo",
 // CHECK-SAME: elements: ![[elements:[0-9]+]]
 // CHECK-SAME: identifier: ".?AUFoo@@"
 
-// CHECK: ![[elements]] = !{![[vshape:[0-9]+]], ![[vptr:[0-9]+]], ![[Nested]], ![[f:[0-9]+]], ![[g:[0-9]+]], ![[h:[0-9]+]]}
+// CHECK: ![[elements]] = !{![[vshape:[0-9]+]], ![[vptr:[0-9]+]], ![[Nested]], ![[f:[0-9]+]], ![[g:[0-9]+]], ![[h:[0-9]+]], ![[i:[0-9]+]]}
 
 // CHECK: ![[vshape]] = !DIDerivedType(tag: DW_TAG_pointer_type, name: "__vtbl_ptr_type", baseType: null, size: 96)
 
 // CHECK: ![[vptr]] = !DIDerivedType(tag: DW_TAG_member, name: "_vptr$Foo",
 // CHECK-SAME: baseType: ![[vptr_ty:[0-9]+]],
 
 // CHECK: ![[vptr_ty]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[vshape]], size: 32
 
 // CHECK: ![[f]] = !DISubprogram(name: "f",
 // CHECK-SAME: containingType: ![[Foo]], virtuality: DW_VIRTUALITY_virtual, virtualIndex: 0,
 // CHECK-SAME: flags: DIFlagPrototyped | DIFlagIntroducedVirtual,
 
 // CHECK: ![[g]] = !DISubprogram(name: "g",
 // CHECK-SAME: containingType: ![[Foo]], virtuality: DW_VIRTUALITY_virtual, virtualIndex: 1,
 // CHECK-SAME: flags: DIFlagPrototyped | DIFlagIntroducedVirtual,
 
 // CHECK: ![[h]] = !DISubprogram(name: "h",
 // CHECK-SAME: containingType: ![[Foo]], virtuality: DW_VIRTUALITY_virtual, virtualIndex: 2,
 // CHECK-SAME: flags: DIFlagPrototyped | DIFlagIntroducedVirtual,
+
+// CHECK: ![[i]] = !DISubprogram(name: "i",
+// CHECK-SAME: flags: DIFlagPrototyped | DIFlagStaticMember
+// CHECK-NEXT: ![[dummy:[0-9]+]] = !DISubroutineType(types: ![[Signature:[0-9]+]])
+// CHECK: ![[Signature]] = !{null, ![[BasicInt:[0-9]+]], ![[BasicInt]]}
+// CHECK: ![[BasicInt]] = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.h

class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase {
   codeview::TypeIndex lowerTypeBasic(const DIBasicType *Ty);
   codeview::TypeIndex lowerTypePointer(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeMemberPointer(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeModifier(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeFunction(const DISubroutineType *Ty);
   codeview::TypeIndex lowerTypeVFTableShape(const DIDerivedType *Ty);
   codeview::TypeIndex lowerTypeMemberFunction(const DISubroutineType *Ty,
                                               const DIType *ClassTy,
-                                              int ThisAdjustment);
+                                              int ThisAdjustment,
+                                              bool IsStaticMethod);
   codeview::TypeIndex lowerTypeEnum(const DICompositeType *Ty);
   codeview::TypeIndex lowerTypeClass(const DICompositeType *Ty);
   codeview::TypeIndex lowerTypeUnion(const DICompositeType *Ty);
 
   /// Symbol records should point to complete types, but type records should
   /// always point to incomplete types to avoid cycles in the type graph. Only
   /// use this entry point when generating symbol records. The complete and
   /// incomplete type indices only differ for record types. All other types use

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

TypeIndex CodeViewDebug::getMemberFunctionType(const DISubprogram *SP,
   auto I = TypeIndices.find({SP, Class});
   if (I != TypeIndices.end())
     return I->second;
 
   // Make sure complete type info for the class is emitted *after* the member
   // function type, as the complete class type is likely to reference this
   // member function type.
   TypeLoweringScope S(*this);
-  TypeIndex TI =
-      lowerTypeMemberFunction(SP->getType(), Class, SP->getThisAdjustment());
+  const bool IsStaticMethod = (SP->getFlags() & DINode::FlagStaticMember) != 0;
+  TypeIndex TI = lowerTypeMemberFunction(
+      SP->getType(), Class, SP->getThisAdjustment(), IsStaticMethod);
   return recordTypeIndexForDINode(SP, TI, Class);
 }
 
 TypeIndex CodeViewDebug::recordTypeIndexForDINode(const DINode *Node,
                                                   TypeIndex TI,
                                                   const DIType *ClassTy) {
   auto InsertResult = TypeIndices.insert({{Node, ClassTy}, TI});
   (void)InsertResult;
TypeIndex CodeViewDebug::lowerType(const DIType *Ty, const DIType *ClassTy) {
   case dwarf::DW_TAG_volatile_type:
   // TODO: add support for DW_TAG_atomic_type here
     return lowerTypeModifier(cast<DIDerivedType>(Ty));
   case dwarf::DW_TAG_subroutine_type:
     if (ClassTy) {
       // The member function type of a member function pointer has no
       // ThisAdjustment.
       return lowerTypeMemberFunction(cast<DISubroutineType>(Ty), ClassTy,
-                                     /*ThisAdjustment=*/0);
+                                     /*ThisAdjustment=*/0,
+                                     /*IsStaticMethod=*/false);
     }
     return lowerTypeFunction(cast<DISubroutineType>(Ty));
   case dwarf::DW_TAG_enumeration_type:
     return lowerTypeEnum(cast<DICompositeType>(Ty));
   case dwarf::DW_TAG_class_type:
   case dwarf::DW_TAG_structure_type:
     return lowerTypeClass(cast<DICompositeType>(Ty));
   case dwarf::DW_TAG_union_type:
TypeIndex CodeViewDebug::lowerTypeFunction(const DISubroutineType *Ty) {
 
   ProcedureRecord Procedure(ReturnTypeIndex, CC, FunctionOptions::None,
                             ArgTypeIndices.size(), ArgListIndex);
   return TypeTable.writeKnownType(Procedure);
 }
 
 TypeIndex CodeViewDebug::lowerTypeMemberFunction(const DISubroutineType *Ty,
                                                  const DIType *ClassTy,
-                                                 int ThisAdjustment) {
+                                                 int ThisAdjustment,
+                                                 bool IsStaticMethod) {
   // Lower the containing class type.
   TypeIndex ClassType = getTypeIndex(ClassTy);
 
   SmallVector<TypeIndex, 8> ReturnAndArgTypeIndices;
   for (DITypeRef ArgTypeRef : Ty->getTypeArray())
     ReturnAndArgTypeIndices.push_back(getTypeIndex(ArgTypeRef));
 
   TypeIndex ReturnTypeIndex = TypeIndex::Void();
   ArrayRef<TypeIndex> ArgTypeIndices = None;
   if (!ReturnAndArgTypeIndices.empty()) {
     auto ReturnAndArgTypesRef = makeArrayRef(ReturnAndArgTypeIndices);
     ReturnTypeIndex = ReturnAndArgTypesRef.front();
     ArgTypeIndices = ReturnAndArgTypesRef.drop_front();
   }
   TypeIndex ThisTypeIndex = TypeIndex::Void();
-  if (!ArgTypeIndices.empty()) {
+  if (!IsStaticMethod && !ArgTypeIndices.empty()) {
     ThisTypeIndex = ArgTypeIndices.front();
     ArgTypeIndices = ArgTypeIndices.drop_front();
   }
 
   ArgListRecord ArgListRec(TypeRecordKind::ArgList, ArgTypeIndices);
   TypeIndex ArgListIndex = TypeTable.writeKnownType(ArgListRec);
 
   CallingConvention CC = dwarfCCToCodeView(Ty->getCC());
 
-  // TODO: Need to use the correct values for:
-  //       FunctionOptions
-  //       ThisPointerAdjustment.
+  // TODO: Need to use the correct values for FunctionOptions.
   MemberFunctionRecord MFR(ReturnTypeIndex, ClassType, ThisTypeIndex, CC,
                            FunctionOptions::None, ArgTypeIndices.size(),
                            ArgListIndex, ThisAdjustment);
   TypeIndex TI = TypeTable.writeKnownType(MFR);
 
   return TI;
 }
 
static MethodOptions translateMethodOptionFlags(const DISubprogram *SP) {
 
   // FIXME: Handle other MethodOptions.
 
   return MethodOptions::None;
 }
 
 static MethodKind translateMethodKindFlags(const DISubprogram *SP,
                                            bool Introduced) {
+  if (SP->getFlags() & DINode::FlagStaticMember)
+    return MethodKind::Static;
+
   switch (SP->getVirtuality()) {
   case dwarf::DW_VIRTUALITY_none:
     break;
   case dwarf::DW_VIRTUALITY_virtual:
     return Introduced ? MethodKind::IntroducingVirtual : MethodKind::Virtual;
   case dwarf::DW_VIRTUALITY_pure_virtual:
     return Introduced ? MethodKind::PureIntroducingVirtual
                       : MethodKind::PureVirtual;
   default:
     llvm_unreachable("unhandled virtuality case");
   }
 
-  // FIXME: Get Clang to mark DISubprogram as static and do something with it.
-
   return MethodKind::Vanilla;
 }
 
 static TypeRecordKind getRecordKind(const DICompositeType *Ty) {
   switch (Ty->getTag()) {
   case dwarf::DW_TAG_class_type:     return TypeRecordKind::Class;
   case dwarf::DW_TAG_structure_type: return TypeRecordKind::Struct;
   }

llvm/trunk/test/DebugInfo/COFF/static-methods.ll

+; RUN: llc < %s -filetype=obj | llvm-readobj - -codeview | FileCheck %s
+
+; Check for the appropriate MethodKind below.
+
+; C++ source used to generate IR:
+; $ cat t.cpp
+; struct A {
+;   static void f();
+;   void g();
+;   static void h(int x, int y);
+; };
+; A *p = new A;
+; $ clang t.cpp -S -emit-llvm -g -gcodeview -o t.ll
+
+; CHECK:      OneMethod {
+; CHECK-NEXT:   TypeLeafKind: LF_ONEMETHOD (0x1511)
+; CHECK-NEXT:   AccessSpecifier: Public (0x3)
+; CHECK-NEXT:   MethodKind: Static (0x2)
+; CHECK-NEXT:   Type: void A::() ({{.*}})
+; CHECK-NEXT:   Name: f
+; CHECK-NEXT: }
+; CHECK-NEXT: OneMethod {
+; CHECK-NEXT:   TypeLeafKind: LF_ONEMETHOD (0x1511)
+; CHECK-NEXT:   AccessSpecifier: Public (0x3)
+; CHECK-NEXT:   Type: void A::() ({{.*}})
+; CHECK-NEXT:   Name: g
+; CHECK-NEXT: }
+; CHECK-NEXT: OneMethod {
+; CHECK-NEXT:   TypeLeafKind: LF_ONEMETHOD (0x1511)
+; CHECK-NEXT:   AccessSpecifier: Public (0x3)
+; CHECK-NEXT:   MethodKind: Static (0x2)
+; CHECK-NEXT:   Type: void A::(int, int) ({{.*}})
+; CHECK-NEXT:   Name: h
+; CHECK-NEXT: }
+
+; ModuleID = 't.cpp'
+source_filename = "t.cpp"
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc19.0.24215"
+
+%struct.A = type { i8 }
+
+@"\01?p@@3PAUA@@A" = global %struct.A* null, align 4, !dbg !0
[e-mail]_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_t.cpp, i8* null }]
+
+; Function Attrs: noinline
+define internal void @"\01??__Ep@@YAXXZ"() #0 !dbg !25 {
+entry:
+  %call = call i8* @"\01??2@YAPAXI@Z"(i32 1) #2, !dbg !26
+  %0 = bitcast i8* %call to %struct.A*, !dbg !26
+  store %struct.A* %0, %struct.A** @"\01?p@@3PAUA@@A", align 4, !dbg !26
+  ret void, !dbg !27
+}
+
+; Function Attrs: nobuiltin
+declare noalias i8* @"\01??2@YAPAXI@Z"(i32) #1
+
+; Function Attrs: noinline
+define internal void @_GLOBAL__sub_I_t.cpp() #0 !dbg !28 {
+entry:
+  call void @"\01??__Ep@@YAXXZ"(), !dbg !30
+  ret void
+}
+
+attributes #0 = { noinline "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nobuiltin "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { builtin }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!20, !21, !22, !23}
+!llvm.ident = !{!24}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "p", linkageName: "\01?p@@3PAUA@@A", scope: !2, file: !3, line: 7, type: !6, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang version 6.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)
+!3 = !DIFile(filename: "t.cpp", directory: "D:\5Csrc\5Ct", checksumkind: CSK_MD5, checksum: "168f4e5caded7d526f64a37783785c64")
+!4 = !{}
+!5 = !{!0}
+!6 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 32)
+!7 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "A", file: !3, line: 1, size: 8, elements: !8, identifier: ".?AUA@@")
+!8 = !{!9, !12, !16}
+!9 = !DISubprogram(name: "f", linkageName: "\01?f@A@@SAXXZ", scope: !7, file: !3, line: 2, type: !10, isLocal: false, isDefinition: false, scopeLine: 2, flags: DIFlagPrototyped | DIFlagStaticMember, isOptimized: false)
+!10 = !DISubroutineType(types: !11)
+!11 = !{null}
+!12 = !DISubprogram(name: "g", linkageName: "\01?g@A@@QAEXXZ", scope: !7, file: !3, line: 3, type: !13, isLocal: false, isDefinition: false, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false)
+!13 = !DISubroutineType(cc: DW_CC_BORLAND_thiscall, types: !14)
+!14 = !{null, !15}
+!15 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 32, flags: DIFlagArtificial | DIFlagObjectPointer)
+!16 = !DISubprogram(name: "h", linkageName: "\01?h@A@@SAXHH@Z", scope: !7, file: !3, line: 4, type: !17, isLocal: false, isDefinition: false, scopeLine: 4, flags: DIFlagPrototyped | DIFlagStaticMember, isOptimized: false)
+!17 = !DISubroutineType(types: !18)
+!18 = !{null, !19, !19}
+!19 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!20 = !{i32 1, !"NumRegisterParameters", i32 0}
+!21 = !{i32 2, !"CodeView", i32 1}
+!22 = !{i32 2, !"Debug Info Version", i32 3}
+!23 = !{i32 1, !"wchar_size", i32 2}
+!24 = !{!"clang version 6.0.0 "}
+!25 = distinct !DISubprogram(name: "??__Ep@@YAXXZ", scope: !3, file: !3, line: 7, type: !10, isLocal: true, isDefinition: true, scopeLine: 7, flags: DIFlagPrototyped, isOptimized: false, unit: !2, variables: !4)
+!26 = !DILocation(line: 7, column: 8, scope: !25)
+!27 = !DILocation(line: 7, column: 12, scope: !25)
+!28 = distinct !DISubprogram(linkageName: "_GLOBAL__sub_I_t.cpp", scope: !3, file: !3, type: !29, isLocal: true, isDefinition: true, flags: DIFlagArtificial, isOptimized: false, unit: !2, variables: !4)
+!29 = !DISubroutineType(types: !4)
+!30 = !DILocation(line: 0, scope: !28)