[LLDB][NativePDB] Global ctor and dtor should be global decls.

[zequanwu]

Summary

This fixes a crash that mistaken global ctor/dtor as funciton methods.

[rnk]

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

1202

Should the check be done before GetParentDeclContext or be done inside GetParentDeclContext so that we don't do wasted work computing a parent that won't be used for dynamic initializers?

[zequanwu]

Address comment.

[labath]

I believe that this fixes the crash, but the names of generated functions still look fairly weird. Could we create them using their mangled name instead? That way, someone might actually call them, if he was so inclined.

[zequanwu]

Update test case after moving the check into CreateDeclInfoForUndecoratedName.

[zequanwu]

In D133446#3779600, @labath wrote:

I believe that this fixes the crash, but the names of generated functions still look fairly weird. Could we create them using their mangled name instead? That way, someone might actually call them, if he was so inclined.

It looks like they don't have mangled name stored in pdb.

[labath]

In D133446#3780961, @zequanwu wrote:

In D133446#3779600, @labath wrote:

I believe that this fixes the crash, but the names of generated functions still look fairly weird. Could we create them using their mangled name instead? That way, someone might actually call them, if he was so inclined.

It looks like they don't have mangled name stored in pdb.

Hmm.. That's unfortunate. In that case, I'm wondering if this shouldn't be handled somehow directly inside MSVCUndecoratedNameParser. What does the function return right now? Do you think that result is going to be useful for other users of that class?

Also, I'm still continuing to be amazed (and scared) by the amount of name parsing that is going on here. Have you checked that there's no better way to associate an entity with its enclosing context?

[zequanwu]

In D133446#3786561, @labath wrote:

In D133446#3780961, @zequanwu wrote:

In D133446#3779600, @labath wrote:

I believe that this fixes the crash, but the names of generated functions still look fairly weird. Could we create them using their mangled name instead? That way, someone might actually call them, if he was so inclined.

It looks like they don't have mangled name stored in pdb.

Hmm.. That's unfortunate. In that case, I'm wondering if this shouldn't be handled somehow directly inside MSVCUndecoratedNameParser. What does the function return right now? Do you think that result is going to be useful for other users of that class?

MSVCUndecoratedNameParser expects a undercoated name of a entity(class, function, variable, etc) and returns the a pair of {pointer to parent decl context, base name of the entity}. This is the only way we use to get enclosing context for a given entity.

Also, I'm still continuing to be amazed (and scared) by the amount of name parsing that is going on here. Have you checked that there's no better way to associate an entity with its enclosing context?

If the entity is a global variable, we don't have any other ways to know if its enclosing context is record or namespace. If the entity is record or function, then we can know if its enclosing context is a record by looking at its type info but need to fall back to parse name again if it's inside a namespace. The issue is there isn't a way to represent namespaces in PDB, and they are just embedded into the name strings.

[labath]

In D133446#3787362, @zequanwu wrote:

In D133446#3786561, @labath wrote:

In D133446#3780961, @zequanwu wrote:

In D133446#3779600, @labath wrote:

I believe that this fixes the crash, but the names of generated functions still look fairly weird. Could we create them using their mangled name instead? That way, someone might actually call them, if he was so inclined.

It looks like they don't have mangled name stored in pdb.

Hmm.. That's unfortunate. In that case, I'm wondering if this shouldn't be handled somehow directly inside MSVCUndecoratedNameParser. What does the function return right now? Do you think that result is going to be useful for other users of that class?

MSVCUndecoratedNameParser expects a undercoated name of a entity(class, function, variable, etc) and returns the a pair of {pointer to parent decl context, base name of the entity}.

Hmm... are you sure about that? CreateDeclInfoForUndecoratedName returns a {decl ctx, base name} pair. MSVCUndecoratedNameParser returns (creates) an array of MSVCUndecoratedNameSpecifiers. My question was about what those specifiers are specifically for the inputs we're talking about here (static void B::dynamic atexit destructor for 'glob'() or such), and whether those outputs "make sense" for some user of that class (it's clear that they don't make sense here).

Basically, I'm trying to see whether it's possible (desirable?) to move this logic into the parser class. The parser already does a lot of string manipulation, so checking for these strings there would make more sense to me than doing some sort of a pre-parse from the outside.

Also, I'm still continuing to be amazed (and scared) by the amount of name parsing that is going on here. Have you checked that there's no better way to associate an entity with its enclosing context?

If the entity is a global variable, we don't have any other ways to know if its enclosing context is record or namespace. If the entity is record or function, then we can know if its enclosing context is a record by looking at its type info but need to fall back to parse name again if it's inside a namespace. The issue is there isn't a way to represent namespaces in PDB, and they are just embedded into the name strings.

Hmm.. well, that's unfortunate, but I guess we'll have to live with it.

[zequanwu]

In D133446#3788956, @labath wrote:

In D133446#3787362, @zequanwu wrote:

In D133446#3786561, @labath wrote:

In D133446#3780961, @zequanwu wrote:

In D133446#3779600, @labath wrote:

I believe that this fixes the crash, but the names of generated functions still look fairly weird. Could we create them using their mangled name instead? That way, someone might actually call them, if he was so inclined.

It looks like they don't have mangled name stored in pdb.

Hmm.. That's unfortunate. In that case, I'm wondering if this shouldn't be handled somehow directly inside MSVCUndecoratedNameParser. What does the function return right now? Do you think that result is going to be useful for other users of that class?

MSVCUndecoratedNameParser expects a undercoated name of a entity(class, function, variable, etc) and returns the a pair of {pointer to parent decl context, base name of the entity}.

Hmm... are you sure about that? CreateDeclInfoForUndecoratedName returns a {decl ctx, base name} pair. MSVCUndecoratedNameParser returns (creates) an array of MSVCUndecoratedNameSpecifiers. My question was about what those specifiers are specifically for the inputs we're talking about here (static void B::dynamic atexit destructor for 'glob'() or such), and whether those outputs "make sense" for some user of that class (it's clear that they don't make sense here).

Basically, I'm trying to see whether it's possible (desirable?) to move this logic into the parser class. The parser already does a lot of string manipulation, so checking for these strings there would make more sense to me than doing some sort of a pre-parse from the outside.

Sorry, I misunderstood you and copied the wrong function name there. And yeah, I think the logic should be moved into MSVCUndecoratedNameParser.

[zequanwu]

Move the logic into MSVCUndecoratedNameParser.

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

clang::DeclContext *PdbAstBuilder::GetOrCreateDeclContextForUid(PdbSymUid uid) {
   if (!decl)
     return nullptr;
 
   return clang::Decl::castToDeclContext(decl);
 }
 
 std::pair<clang::DeclContext *, std::string>
 PdbAstBuilder::CreateDeclInfoForUndecoratedName(llvm::StringRef name) {
+  clang::DeclContext *context =
+      FromCompilerDeclContext(GetTranslationUnitDecl());
+  // Global ctor and dtor are global decls.
+  if (name.contains("dynamic initializer for") ||
+      name.contains("dynamic atexit destructor for"))
+    return {context, std::string(name)};
   MSVCUndecoratedNameParser parser(name);
   llvm::ArrayRef<MSVCUndecoratedNameSpecifier> specs = parser.GetSpecifiers();
 
-  auto context = FromCompilerDeclContext(GetTranslationUnitDecl());
-
   llvm::StringRef uname = specs.back().GetBaseName();
   specs = specs.drop_back();
   if (specs.empty())
     return {context, std::string(name)};
 
   llvm::StringRef scope_name = specs.back().GetFullName();
 
   // It might be a class name, try that first.
return CreateFunctionDecl(func_sid, func_name, func_ti, func_ct, param_count,
                             clang::SC_None, true, parent);
 }
 
 clang::FunctionDecl *
 PdbAstBuilder::GetOrCreateFunctionDecl(PdbCompilandSymId func_id) {
   if (clang::Decl *decl = TryGetDecl(func_id))
     return llvm::dyn_cast<clang::FunctionDecl>(decl);
 
   clang::DeclContext *parent = GetParentDeclContext(PdbSymUid(func_id));
   if (!parent)
     return nullptr;
   std::string context_name;
   if (clang::NamespaceDecl *ns = llvm::dyn_cast<clang::NamespaceDecl>(parent)) {
     context_name = ns->getQualifiedNameAsString();
   } else if (clang::TagDecl *tag = llvm::dyn_cast<clang::TagDecl>(parent)) {
     context_name = tag->getQualifiedNameAsString();
   }
PdbAstBuilder::GetOrCreateFunctionDecl(PdbCompilandSymId func_id) {
   const clang::FunctionProtoType *func_type =
       llvm::dyn_cast<clang::FunctionProtoType>(qt);
 
   CompilerType func_ct = ToCompilerType(qt);
 
   llvm::StringRef proc_name = proc.Name;
   proc_name.consume_front(context_name);
   proc_name.consume_front("::");
-
   clang::FunctionDecl *function_decl =
       CreateFunctionDecl(func_id, proc_name, proc.FunctionType, func_ct,
                          func_type->getNumParams(), storage, false, parent);
   if (function_decl == nullptr)
     return nullptr;
 
   lldbassert(m_uid_to_decl.count(toOpaqueUid(func_id)) == 0);
   m_uid_to_decl[toOpaqueUid(func_id)] = function_decl;

lldb/test/Shell/SymbolFile/NativePDB/global-ctor-dtor.cpp

+// clang-format off
+// REQUIRES: lld, x86
+
+// Global ctor and dtor should be globals decls.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -fno-addrsig -c /Fo%t.obj -- %s
+// RUN: lld-link -opt:icf -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb -force
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | FileCheck %s
+
+struct A {
+  ~A() {};
+};
+struct B {
+  static A glob;
+};
+
+A B::glob = A();
+int main() {
+  return 0;
+}
+
+// CHECK:      struct B {
+// CHECK-NEXT:     static A glob;
+// CHECK-NEXT: };
+// CHECK-NEXT: static void `dynamic initializer for 'glob'();
+// CHECK-NEXT: static void `dynamic atexit destructor for 'glob'();
+// CHECK-NEXT: int main();
+// CHECK-NEXT: static void _GLOBAL__sub_I_global_ctor_dtor.cpp();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT:     ~A();
+// CHECK-NEXT: };