Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
pcc updated this revision to Diff 59260.
pcc marked 2 inline comments as done.
pcc added a comment.
- Address review comments
http://reviews.llvm.org/D20415
Files:
lib/CodeGen/CGDebugInfo.cpp
lib/CodeGen/CGDebugInfo.h
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/2009-10-20-GlobalDebug.c
test/CodeGen/2010-08-10-DbgConstant.c
test/CodeGen/debug-info-packed-struct.c
test/CodeGen/debug-info-static.c
test/CodeGenCXX/debug-info-access.cpp
test/CodeGenCXX/debug-info-alias.cpp
test/CodeGenCXX/debug-info-anon-namespace.cpp
test/CodeGenCXX/debug-info-anon-union-vars.cpp
test/CodeGenCXX/debug-info-method.cpp
test/CodeGenCXX/debug-info-namespace.cpp
test/CodeGenCXX/debug-info-static-member.cpp
test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
test/CodeGenCXX/debug-info-template-member.cpp
test/CodeGenCXX/debug-info-template.cpp
test/CodeGenCXX/debug-info-uuid.cpp
test/CodeGenCXX/debug-info.cpp
test/CodeGenCXX/debug-lambda-expressions.cpp
test/CodeGenCXX/inline-dllexport-member.cpp
test/Driver/darwin-debug-flags.c
test/Modules/ExtDebugInfo.cpp
test/Modules/ExtDebugInfo.m
Index: test/Modules/ExtDebugInfo.m
===
--- test/Modules/ExtDebugInfo.m
+++ test/Modules/ExtDebugInfo.m
@@ -34,14 +34,8 @@
return [c property];
}
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClassWithPrivateIVars",
-// CHECK-SAME: flags: DIFlagObjcClassComplete
-
// CHECK: ![[MOD:.*]] = !DIModule(scope: null, name: "DebugObjC
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "hidden_ivar",
-// CHECK-SAME: flags: DIFlagPrivate)
-
// CHECK: !DIGlobalVariable(name: "GlobalUnion",
// CHECK-SAME: type: ![[GLOBAL_UNION:[0-9]+]]
// CHECK: ![[GLOBAL_UNION]] = distinct !DICompositeType(tag: DW_TAG_union_type,
@@ -52,10 +46,11 @@
// CHECK: ![[GLOBAL_STRUCT]] = distinct !DICompositeType(tag: DW_TAG_structure_type,
// CHECK-SAME:elements: !{{[0-9]+}})
-// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "TypedefUnion",
-// CHECK-SAME: baseType: ![[TD_UNION:.*]])
-// CHECK: ![[TD_UNION]] = !DICompositeType(tag: DW_TAG_union_type,
-// CHECK-SAME: flags: DIFlagFwdDecl)
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClassWithPrivateIVars",
+// CHECK-SAME: flags: DIFlagObjcClassComplete
+
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "hidden_ivar",
+// CHECK-SAME: flags: DIFlagPrivate)
// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "TypedefEnum",
// CHECK-SAME: baseType: ![[TD_ENUM:.*]])
@@ -67,6 +62,11 @@
// CHECK: ![[TD_STRUCT]] = !DICompositeType(tag: DW_TAG_structure_type,
// CHECK-SAME: flags: DIFlagFwdDecl)
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "TypedefUnion",
+// CHECK-SAME: baseType: ![[TD_UNION:.*]])
+// CHECK: ![[TD_UNION]] = !DICompositeType(tag: DW_TAG_union_type,
+// CHECK-SAME: flags: DIFlagFwdDecl)
+
// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClass",
// CHECK-SAME: scope: ![[MOD]],
// CHECK-SAME: flags: DIFlagFwdDecl)
Index: test/Modules/ExtDebugInfo.cpp
===
--- test/Modules/ExtDebugInfo.cpp
+++ test/Modules/ExtDebugInfo.cpp
@@ -67,20 +67,14 @@
anon.i = GlobalStruct.i = GlobalUnion.i = GlobalEnum;
}
-
-// CHECK: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Struct",
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Enum",
// CHECK-SAME: scope: ![[NS:[0-9]+]],
// CHECK-SAME: flags: DIFlagFwdDecl,
-// CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE")
+// CHECK-SAME: identifier: "_ZTSN8DebugCXX4EnumE")
// CHECK: ![[NS]] = !DINamespace(name: "DebugCXX", scope: ![[MOD:[0-9]+]],
// CHECK: ![[MOD]] = !DIModule(scope: null, name: {{.*}}DebugCXX
-// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Enum",
-// CHECK-SAME: scope: ![[NS]],
-// CHECK-SAME: flags: DIFlagFwdDecl,
-// CHECK-SAME: identifier: "_ZTSN8DebugCXX4EnumE")
-
// This type is anchored in the module by an explicit template instantiation.
// CHECK: !DICompositeType(tag: DW_TAG_class_type,
// CHECK-SAME: name: "Template",
@@ -119,7 +113,12 @@
// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "static_member",
-// CHECK-SAME: scope: ![[STRUCT]]
+// CHECK-SAME: scope: ![[STRUCT:[0-9]*]]
+
+// CHECK: ![[STRUCT]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Struct",
+// CHECK-SAME: scope: ![[NS]],
+// CHECK-SAME: flags: DIFlagFwdDecl,
+// CHECK-SAME: identifier:
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3427 @@ -3425,3 +3426,3 @@ DContext, DeclName, LinkageName, Unit, LineNo, getOrCreateType(T, Unit), -Var->hasInternalLinkage(), Var, +Var->hasInternalLinkage(), nullptr, getOrCreateStaticDataMemberDeclarationOrNull(D)); aprantl wrote: > I think it would be more readable to pass an empty DIExpression() here. /* expression=*/ is good enough. Comment at: lib/CodeGen/CGDebugInfo.cpp:3474 @@ -3472,1 +3473,3 @@ return; + llvm::DIExpression *InitExpr = nullptr; + if (Init.isInt()) aprantl wrote: > Shouldn't the default constructor of DIExpression() wrap an MDNode nullptr? Sorry, that was a couple of IR evolution steps ago. DIExpression now inserts from MDNode. http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
aprantl accepted this revision. aprantl added a comment. This revision is now accepted and ready to land. LGTM with small changes. Comment at: lib/CodeGen/CGDebugInfo.cpp:3427 @@ -3425,3 +3426,3 @@ DContext, DeclName, LinkageName, Unit, LineNo, getOrCreateType(T, Unit), -Var->hasInternalLinkage(), Var, +Var->hasInternalLinkage(), nullptr, getOrCreateStaticDataMemberDeclarationOrNull(D)); I think it would be more readable to pass an empty DIExpression() here. Comment at: lib/CodeGen/CGDebugInfo.cpp:3474 @@ -3472,1 +3473,3 @@ return; + llvm::DIExpression *InitExpr = nullptr; + if (Init.isInt()) Shouldn't the default constructor of DIExpression() wrap an MDNode nullptr? Comment at: lib/CodeGen/CGDebugInfo.h:348 @@ -347,2 +347,3 @@ /// Emit global variable's debug info. + void EmitGlobalVariable(const ValueDecl *VD, const APValue ); While we're here: Emit *a* global variable's debug info? http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
pcc added a comment. Ping. http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3477 @@ -3473,1 +3476,3 @@ +InitExpr = +DBuilder.createConstantValueExpression(Init.getInt().getExtValue()); GV.reset(DBuilder.createGlobalVariable( pcc wrote: > aprantl wrote: > > pcc wrote: > > > aprantl wrote: > > > > Are we regressing floating point constants here? > > > It looks like we never really supported floating point constants in the > > > DWARF output. I can only see support for integer constants: > > > http://llvm-cs.pcc.me.uk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#192 > > Looking at http://llvm-cs.pcc.me.uk/test/DebugInfo/X86/float_const.ll we at > > least do to some degree :-) > That isn't a global variable test case though, is it? Ah that's right. We didn't support global float constants. http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
pcc added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3477 @@ -3473,1 +3476,3 @@ +InitExpr = +DBuilder.createConstantValueExpression(Init.getInt().getExtValue()); GV.reset(DBuilder.createGlobalVariable( aprantl wrote: > pcc wrote: > > aprantl wrote: > > > Are we regressing floating point constants here? > > It looks like we never really supported floating point constants in the > > DWARF output. I can only see support for integer constants: > > http://llvm-cs.pcc.me.uk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#192 > Looking at http://llvm-cs.pcc.me.uk/test/DebugInfo/X86/float_const.ll we at > least do to some degree :-) That isn't a global variable test case though, is it? http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3477 @@ -3473,1 +3476,3 @@ +InitExpr = +DBuilder.createConstantValueExpression(Init.getInt().getExtValue()); GV.reset(DBuilder.createGlobalVariable( pcc wrote: > aprantl wrote: > > Are we regressing floating point constants here? > It looks like we never really supported floating point constants in the DWARF > output. I can only see support for integer constants: > http://llvm-cs.pcc.me.uk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#192 Looking at http://llvm-cs.pcc.me.uk/test/DebugInfo/X86/float_const.ll we at least do to some degree :-) http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
pcc added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3477 @@ -3473,1 +3476,3 @@ +InitExpr = +DBuilder.createConstantValueExpression(Init.getInt().getExtValue()); GV.reset(DBuilder.createGlobalVariable( aprantl wrote: > Are we regressing floating point constants here? It looks like we never really supported floating point constants in the DWARF output. I can only see support for integer constants: http://llvm-cs.pcc.me.uk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#192 http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3393 @@ +3392,3 @@ +DContext, FieldName, LinkageName, Unit, LineNo, FieldTy, +Var->hasInternalLinkage(), nullptr, nullptr); +Var->addDebugInfo(GV); aprantl wrote: > Is there a good reason for not changing the DIBuilder interface to drop the > global field? Sorry about the confusion, I should have read the other patch first. This has morphed into the DIExpression. That's fine! http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
aprantl added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3393 @@ +3392,3 @@ +DContext, FieldName, LinkageName, Unit, LineNo, FieldTy, +Var->hasInternalLinkage(), nullptr, nullptr); +Var->addDebugInfo(GV); Is there a good reason for not changing the DIBuilder interface to drop the global field? Comment at: lib/CodeGen/CGDebugInfo.cpp:3477 @@ -3473,1 +3476,3 @@ +InitExpr = +DBuilder.createConstantValueExpression(Init.getInt().getExtValue()); GV.reset(DBuilder.createGlobalVariable( Are we regressing floating point constants here? http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20415: Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
pcc planned changes to this revision. pcc added a comment. One thing that I forgot to do here was to add a test covering my changes to `CGDebugInfo::EmitGlobalVariable`. I'll do that momentarily. http://reviews.llvm.org/D20415 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
