[DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types

[akhuang]

Summary

In line-tables-only mode, we used to emit qualified names as the display name for functions when using CodeView.
This patch changes to emitting the parent scopes instead, with forward declarations for class types.
Also remove the linkage name in line-tables-only to save size.
The total object file size ends up being slightly smaller than if we use the full qualified names.

[dblaikie]

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

[akhuang]

In D94639#2496892, @dblaikie wrote:

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

This was kind of motivated by the issue in https://bugs.llvm.org/show_bug.cgi?id=48432, even though this patch doesn't solve that particular bug. I guess the general issue is that in CV the only way to make the functions unique is by their display name or type. So previously we were going in the direction of differentiating functions by the display name (like using the qualified name), but it seems like it's more size-efficient to use types instead, which is what this patch does.

[dblaikie]

In D94639#2496923, @akhuang wrote:

In D94639#2496892, @dblaikie wrote:

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?
It'd be a bit unfortunate if we have divergence in -gmlt behavior between DWARF and CodeView due to different usage requirements, rather than format requirements - gmlt in DWARF I think uses only the unqualified name, though eventually combined with -fdebug-info-for-profiling which put the mangled name on such functions to make it clear for profilers. (though the unqualified name only behavior is still handy for the sanitizers - where the unqualified/ambiguous name is a "good enough" point for size V accuracy)

This was kind of motivated by the issue in https://bugs.llvm.org/show_bug.cgi?id=48432, even though this patch doesn't solve that particular bug. I guess the general issue is that in CV the only way to make the functions unique is by their display name or type. So previously we were going in the direction of differentiating functions by the display name (like using the qualified name), but it seems like it's more size-efficient to use types instead, which is what this patch does.

OK - thanks for walking me through it, that sounds good to me.

Might be worth rephrasing from "emit type information for function scopes" to "emit parent/context scopes for functions, using declarations for any scopes that are types". (my own fault, but reading this quickly, I assumed it was about emitting type information for the parameters (DWARF gmlt drops the parameters/types))

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

[akhuang]

In D94639#2496950, @dblaikie wrote:

In D94639#2496923, @akhuang wrote:

In D94639#2496892, @dblaikie wrote:

What parts of this are motivated by CodeView requirements (do functions have to have unique names in CV?)?

Might be worth rephrasing from "emit type information for function scopes" to "emit parent/context scopes for functions, using declarations for any scopes that are types". (my own fault, but reading this quickly, I assumed it was about emitting type information for the parameters (DWARF gmlt drops the parameters/types))

That sounds good

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

Yep, this patch doesn't deal with overloading or lambdas. For lambdas I have a separate patch, and for overloading I haven't really thought much about yet. Seems like we'd have to include all the parameter type info somehow (or include all the parameters in the display name).

[rnk]

In D94639#2496969, @akhuang wrote:

In D94639#2496950, @dblaikie wrote:

How does any of this deal with overloading? I guess for either solution (qualified name or real scopes) you have to include all the parameter type info too to avoid the functions being treated as identical/duplicate by CV?

Yep, this patch doesn't deal with overloading or lambdas. For lambdas I have a separate patch, and for overloading I haven't really thought much about yet. Seems like we'd have to include all the parameter type info somehow (or include all the parameters in the display name).

Yes, I encouraged Amy to approach this incrementally:

• emit forward declarations for types used in function scopes (this patch)
• make lambda type names more unique (next)
• try discarding the linkage name from our type info to save size
• add overload type information

In particular, I wanted to measure the size impact of adding function type info in isolation. I suspect that the type info we need to make overload signatures unique is very small. My theory is that most debug info size comes from complete descriptions of class types.

[rnk]

clang/lib/CodeGen/CGDebugInfo.cpp

1050–1052

I see Amy included the linkage name step as part of this patch, so disregard that comment.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

370–373 ↗

(On Diff #316525)

Do we still need the LLVM part of this? We needed it before because we were operating on strings like foo::<unnamed-tag>::operator(). Can we leave it as is for now, or handle it as a separate patch?

[dblaikie]

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

I'll leave the rest of the review/final approval up to @rnk

[akhuang]

clang/lib/CodeGen/CGDebugInfo.cpp

1050–1052

oops, didn't realize that I included it here. Anyway, I can commit this separately.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

370–373 ↗

(On Diff #316525)

Oh, right. I don't think we need this anymore..

[akhuang]

Remove llvm change.

[rnk]

lgtm

In D94639#2497031, @dblaikie wrote:

(I'm generally good with this - llvm & clang changes should be committed separately & maybe the linkage part too)

I'll leave the rest of the review/final approval up to @rnk

Thanks for checking the direction, David, I felt it was worth getting your input.

[SeTSeR]

Hello! As far as I understand, Clang does not preserve info about namespace for functions declared in namespaces with -gline-numbers-only flag. Is there any way to retrieve this info for DWARF? I thought FIXME at line 261 in CGDebugInfo was addressing exactly this issue.

[rnk]

In D94639#2917340, @SeTSeR wrote:

Hello! As far as I understand, Clang does not preserve info about namespace for functions declared in namespaces with -gline-numbers-only flag. Is there any way to retrieve this info for DWARF? I thought FIXME at line 261 in CGDebugInfo was addressing exactly this issue.

No, I don't think there is a way to achieve this.

The current -gcodeview -gline-tables-only behavior as I understand it is to emit regular debug info, but skip two key source of info:

• never emit complete class declarations, always use forward decls
• never emit variable locations, only emit source locations

For DWARF, I think we also try to skip namespace scopes, class forward decls, function types, and other things like that, but I forget the details.

I happen to know that many users of -gline-tables-only are very space constrained: we can't simply port this change from CV to DWARF. We'd have to add a new flag. The bar for such a flag is high. If someone provides a well-motivated use case, we could consider it.

[SeTSeR]

In D94639#2917445, @rnk wrote:

In D94639#2917340, @SeTSeR wrote:

Hello! As far as I understand, Clang does not preserve info about namespace for functions declared in namespaces with -gline-numbers-only flag. Is there any way to retrieve this info for DWARF? I thought FIXME at line 261 in CGDebugInfo was addressing exactly this issue.

No, I don't think there is a way to achieve this.

The current -gcodeview -gline-tables-only behavior as I understand it is to emit regular debug info, but skip two key source of info:

• never emit complete class declarations, always use forward decls
• never emit variable locations, only emit source locations

For DWARF, I think we also try to skip namespace scopes, class forward decls, function types, and other things like that, but I forget the details.

I happen to know that many users of -gline-tables-only are very space constrained: we can't simply port this change from CV to DWARF. We'd have to add a new flag. The bar for such a flag is high. If someone provides a well-motivated use case, we could consider it.

Thank you for the detailed answer. I'll discuss our use case with our team. Should I create a separate ticket for this? Or maybe it would be better if I submitted the PR adding this flag?

[rnk]

In D94639#2918559, @SeTSeR wrote:

Thank you for the detailed answer. I'll discuss our use case with our team. Should I create a separate ticket for this? Or maybe it would be better if I submitted the PR adding this flag?

It's free to upload patches, and they help drive discussion forward, but I think the main thing is to be really clear about what problem this solves and what the costs are. An easy metric for the cost would be the binary size of clang with -g1 and how much it increases in the new mode.

clang/lib/CodeGen/CGDebugInfo.cpp

 }
 
 StringRef CGDebugInfo::getFunctionName(const FunctionDecl *FD) {
   assert(FD && "Invalid FunctionDecl!");
   IdentifierInfo *FII = FD->getIdentifier();
   FunctionTemplateSpecializationInfo *Info =
       FD->getTemplateSpecializationInfo();
 
-  // Emit the unqualified name in normal operation. LLVM and the debugger can
-  // compute the fully qualified name from the scope chain. If we're only
-  // emitting line table info, there won't be any scope chains, so emit the
-  // fully qualified name here so that stack traces are more accurate.
-  // FIXME: Do this when emitting DWARF as well as when emitting CodeView after
-  // evaluating the size impact.
-  bool UseQualifiedName = DebugKind == codegenoptions::DebugLineTablesOnly &&
-                          CGM.getCodeGenOpts().EmitCodeView;
-
-  if (!Info && FII && !UseQualifiedName)
+  if (!Info && FII)
     return FII->getName();
 
   SmallString<128> NS;
   llvm::raw_svector_ostream OS(NS);
-  if (!UseQualifiedName)
-    FD->printName(OS);
-  else
-    FD->printQualifiedName(OS, getPrintingPolicy());
 
   // Add any template specialization args.
   if (Info) {
     const TemplateArgumentList *TArgs = Info->TemplateArguments;
     printTemplateArgumentList(OS, TArgs->asArray(), getPrintingPolicy());
   }
 
   // Copy this name on the side and use its reference.
CGDebugInfo::getOrCreateRecordFwdDecl(const RecordType *Ty,
   // add the flag if a record has no definition because we don't know whether
   // it will be trivial or not.
   if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD))
     if (!CXXRD->hasDefinition() ||
         (CXXRD->hasDefinition() && !CXXRD->isTrivial()))
       Flags |= llvm::DINode::FlagNonTrivial;
 
   // Create the type.
-  SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
+  SmallString<256> Identifier;
+  // Don't include a linkage name in line tables only.
+  if (CGM.getCodeGenOpts().hasReducedDebugInfo())
+    Identifier = getTypeIdentifier(Ty, CGM, TheCU);
   llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
       getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align, Flags,
       Identifier);
   if (CGM.getCodeGenOpts().DebugFwdTemplateParams)
     if (auto *TSpecial = dyn_cast<ClassTemplateSpecializationDecl>(RD))
       DBuilder.replaceArrays(RetTy, llvm::DINodeArray(),
                              CollectCXXTemplateParams(TSpecial, DefUnit));
   ReplaceMap.emplace_back(
static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
                                  const LangOptions &LangOpts) {
   if (DebugTypeExtRefs && isDefinedInClangModule(RD->getDefinition()))
     return true;
 
   if (auto *ES = RD->getASTContext().getExternalSource())
     if (ES->hasExternalDefinitions(RD) == ExternalASTSource::EK_Always)
       return true;
 
+  if (DebugKind == codegenoptions::DebugLineTablesOnly)
+    return true;
+
   if (DebugKind > codegenoptions::LimitedDebugInfo)
     return false;
 
   if (!LangOpts.CPlusPlus)
     return false;
 
   if (!RD->isCompleteDefinitionRequired())
     return true;
void CGDebugInfo::collectFunctionDeclProps(GlobalDecl GD, llvm::DIFile *Unit,
   // subprogram name, no need to have it at all unless coverage is enabled or
   // debug is set to more than just line tables or extra debug info is needed.
   if (LinkageName == Name || (!CGM.getCodeGenOpts().EmitGcovArcs &&
                               !CGM.getCodeGenOpts().EmitGcovNotes &&
                               !CGM.getCodeGenOpts().DebugInfoForProfiling &&
                               DebugKind <= codegenoptions::DebugLineTablesOnly))
     LinkageName = StringRef();
 
-  if (CGM.getCodeGenOpts().hasReducedDebugInfo()) {
+  // Emit the function scope in line tables only mode (if CodeView) to
+  // differentiate between function names.
+  if (CGM.getCodeGenOpts().hasReducedDebugInfo() ||
+      (DebugKind == codegenoptions::DebugLineTablesOnly &&
+       CGM.getCodeGenOpts().EmitCodeView)) {
     if (const NamespaceDecl *NSDecl =
             dyn_cast_or_null<NamespaceDecl>(FD->getDeclContext()))
       FDContext = getOrCreateNamespace(NSDecl);
     else if (const RecordDecl *RDecl =
                  dyn_cast_or_null<RecordDecl>(FD->getDeclContext())) {
       llvm::DIScope *Mod = getParentModuleOrNull(RDecl);
       FDContext = getContextDescriptor(RDecl, Mod ? Mod : TheCU);
     }
+  }
+  if (CGM.getCodeGenOpts().hasReducedDebugInfo()) {
     // Check if it is a noreturn-marked function
     if (FD->isNoReturn())
       Flags |= llvm::DINode::FlagNoReturn;
     // Collect template parameters.
     TParamsArray = CollectFunctionTemplateParams(FD, Unit);
   }
 }
 

clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp

 // RUN:    grep -E 'DISubprogram|DICompositeType' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | \
 // RUN:    FileCheck %s --check-prefix=CHECK --check-prefix=UNQUAL
 
 void freefunc() { }
 // CHECK-DAG: "freefunc"
 
 namespace N {
   int b() { return 0; }
-// UNQUAL-DAG: "b"
-// QUAL-DAG: "N::b"
+// CHECK-DAG: "b"
   namespace { void func() { } }
-// UNQUAL-DAG: "func"
-// QUAL-DAG: "N::`anonymous namespace'::func"
+// CHECK-DAG: "func"
 }
 
 void _c(void) {
   N::func();
 }
 // CHECK-DAG: "_c"
 
 struct foo {
   int operator+(int);
   foo(){}
-// UNQUAL-DAG: "foo"
-// QUAL-DAG: "foo::foo"
+// CHECK-DAG: "foo"
 
   ~foo(){}
-// UNQUAL-DAG: "~foo"
-// QUAL-DAG: "foo::~foo"
+// CHECK-DAG: "~foo"
 
   foo(int i){}
-// UNQUAL-DAG: "foo"
-// QUAL-DAG: "foo::foo"
+// CHECK-DAG: "foo"
 
   foo(char *q){}
-// UNQUAL-DAG: "foo"
-// QUAL-DAG: "foo::foo"
+// CHECK-DAG: "foo"
 
   static foo* static_method() { return 0; }
-// UNQUAL-DAG: "static_method"
-// QUAL-DAG: "foo::static_method"
+// CHECK-DAG: "static_method"
 
 };
 
 void use_foo() {
   foo f1, f2(1), f3((char*)0);
   foo::static_method();
 }
 
-// UNQUAL-DAG: "operator+"
-// QUAL-DAG: "foo::operator+"
+// CHECK-DAG: "operator+"
 int foo::operator+(int a) { return a; }
 
 // PR17371
 struct OverloadedNewDelete {
   // __cdecl
   void *operator new(__SIZE_TYPE__);
   void *operator new[](__SIZE_TYPE__);
   void operator delete(void *);
   void operator delete[](void *);
   // __thiscall
   int operator+(int);
 };
 
 void *OverloadedNewDelete::operator new(__SIZE_TYPE__ s) { return 0; }
 void *OverloadedNewDelete::operator new[](__SIZE_TYPE__ s) { return 0; }
 void OverloadedNewDelete::operator delete(void *) { }
 void OverloadedNewDelete::operator delete[](void *) { }
 int OverloadedNewDelete::operator+(int x) { return x; };
 
-// UNQUAL-DAG: "operator new"
-// UNQUAL-DAG: "operator new[]"
-// UNQUAL-DAG: "operator delete"
-// UNQUAL-DAG: "operator delete[]"
-// UNQUAL-DAG: "operator+"
-// QUAL-DAG: "OverloadedNewDelete::operator new"
-// QUAL-DAG: "OverloadedNewDelete::operator new[]"
-// QUAL-DAG: "OverloadedNewDelete::operator delete"
-// QUAL-DAG: "OverloadedNewDelete::operator delete[]"
-// QUAL-DAG: "OverloadedNewDelete::operator+"
-
+// CHECK-DAG: "operator new"
+// CHECK-DAG: "operator new[]"
+// CHECK-DAG: "operator delete"
+// CHECK-DAG: "operator delete[]"
+// CHECK-DAG: "operator+"
 
 template <typename T, void (*)(void)>
 void fn_tmpl() {}
 
 template void fn_tmpl<int, freefunc>();
 // CHECK-DAG: "fn_tmpl<int,&freefunc>"
 
 template <typename A, typename B, typename C> struct ClassTemplate { A a; B b; C c; };
 ClassTemplate<char, short, ClassTemplate<int, int, int> > f;
 // This will only show up in normal debug builds.  The space in `> >` is
 // important for compatibility with Windows debuggers, so it should always be
 // there when generating CodeView.
 // UNQUAL-DAG: "ClassTemplate<char,short,ClassTemplate<int,int,int> >"

clang/test/CodeGenCXX/debug-info-codeview-scopes.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -debug-info-kind=line-tables-only -S -emit-llvm -std=c++11 -o - %s | FileCheck --check-prefix LINUX %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -debug-info-kind=line-tables-only -gcodeview -S -emit-llvm -std=c++11 -o - %s | FileCheck --check-prefix MSVC %s
+
+// Check that we emit type information for function scopes in line tables for
+// CodeView.
+
+namespace A {
+void f() {}
+
+struct S {
+  static void m() {}
+};
+}
+
+int main() {
+  A::f();
+  A::S::m();
+  return 0;
+  // MSVC:       !{{[0-9]+}} = distinct !DISubprogram(name: "f"
+  // MSVC-SAME:     scope: [[SCOPE1:![0-9]+]]
+  // MSVC-SAME:     )
+  // MSVC:       [[SCOPE1]] = !DINamespace(name: "A", {{.*}})
+  // MSVC:       !{{[0-9]+}} = distinct !DISubprogram(name: "m"
+  // MSVC-SAME:     scope: [[SCOPE2:![0-9]+]]
+  // MSVC-SAME:     )
+  // MSVC:       [[SCOPE2]] = !DICompositeType(tag: DW_TAG_structure_type,
+  // MSVC-SAME:     name: "S",
+  // MSVC-SAME:     scope: [[SCOPE1]]
+  // MSVC-SAME:     )
+
+  // LINUX-NOT: !DINamespace
+  // LINUX-NOT: !DICompositeType
+  return 0;
+}