[NativePDB] Add basic support of methods recostruction in AST

[aleksandr.urakov]

Summary

This patch adds the basic support of methods reconstruction by native PDB plugin. It contains only most obvious changes (it processes LF_ONEMETHOD and LF_METHOD records), some things still remain unsolved:

• mangled names retrieving;
• support of template methods (they are not presented as LF_ONEMETHOD records at all).

This info is contained in the Symbols stream, not in the TPI stream. As far as I understand, we can't find in a simple way the link between a LF_ONEMETHOD record and a corresponding S_GPROC32 record. I think it's the place, where we need to use the approach similar to D54053 (to parse mangled names and to build some part of AST based on this info). That's what I'm planning to implement next. Do you have any objections on this? Or may be there's a better way to solve this?

[zturner]

This info is contained in the Symbols stream, not in the TPI stream. As far as I understand, we can't find in a simple way the link between a LF_ONEMETHOD record and a corresponding S_GPROC32 record. I think it's the place, where we need to use the approach similar to D54053 (to parse mangled names and to build some part of AST based on this info). That's what I'm planning to implement next. Do you have any objections on this? Or may be there's a better way to solve this?

I think it's probably best to skip this part for now and come back to it later. The only thing that will be missing is the ability to use member function templates in expressions. Of course we need this eventually, but probably there is more useful stuff to work on first.

One idea for implementing this though might be to add a pre-processing step of the publics stream similar to how we pre-process the TPI stream. This would also allow us to find mangled names of global functions as well (currently even for global functions, we create an empty Mangled structure when LLDB asks us for the mangled name). One pass over the publics stream should be simpler and more straightforward than a pass over every single module's symbol stream. From there we have the address, which tells us the module, and then we can use the CompilandIndexItem::FindSymbolsByVa() to find the S_GPROC32 record, and from there find the type. But I think this is a large effort for low value, so we should maybe wait until the easier stuff is done first. I haven't looked at the rest of the patch yet, but I will in a little bit. Thanks!

[aleksandr.urakov]

In D56126#1342114, @zturner wrote:

I think it's probably best to skip this part for now and come back to it later. The only thing that will be missing is the ability to use member function templates in expressions. Of course we need this eventually, but probably there is more useful stuff to work on first.

Yes, I agree. So I can continue with virtual bases support (but only since the middle of January), good?

One idea for implementing this though might be to add a pre-processing step of the publics stream similar to how we pre-process the TPI stream. This would also allow us to find mangled names of global functions as well (currently even for global functions, we create an empty Mangled structure when LLDB asks us for the mangled name). One pass over the publics stream should be simpler and more straightforward than a pass over every single module's symbol stream. From there we have the address, which tells us the module, and then we can use the CompilandIndexItem::FindSymbolsByVa() to find the S_GPROC32 record, and from there find the type. But I think this is a large effort for low value, so we should maybe wait until the easier stuff is done first. I haven't looked at the rest of the patch yet, but I will in a little bit. Thanks!

Thanks for comment! I think that such preprocessing is a good idea. But is it guaranteed that we will have a record in the publics stream for any function?

lit/SymbolFile/NativePDB/ast-methods.cpp

33 ↗

(On Diff #179619)

The one thing I've forgot to say about is that parameter names are in Symbols stream too. Is it ok for now to proceed without them too?

[zturner]

lit/SymbolFile/NativePDB/ast-methods.cpp

28 ↗

(On Diff #179619)

I think this is wrong. If I compile this file myself and look at the debug info, the debug info says it has cdecl calling convention.

D:\src\llvmbuild\ninja-x64>bin\llvm-pdbutil.exe dump -types foo.pdb | grep -C 5 simple_method
   0x106D | LF_METHODLIST [size = 20]
            - Method [type = 0x106B, vftable offset = -1, attrs = public compiler-generated]
            - Method [type = 0x106C, vftable offset = -1, attrs = public compiler-generated]
   0x106E | LF_FIELDLIST [size = 152]
            - LF_VFUNCTAB type = 0x1056
            - LF_ONEMETHOD [name = `simple_method`]
              type = 0x1059, vftable offset = -1, attrs = public
            - LF_ONEMETHOD [name = `virtual_method`]
              type = 0x1059, vftable offset = 0, attrs = public intro virtual
            - LF_ONEMETHOD [name = `static_method`]
              type = 0x105A, vftable offset = -1, attrs = public static

D:\src\llvmbuild\ninja-x64>bin\llvm-pdbutil.exe dump -types -type-index=0x1059 foo.pdb


                     Types (TPI Stream)
============================================================
  Showing 1 records.
   0x1059 | LF_MFUNCTION [size = 28]
            return type = 0x0003 (void), # args = 0, param list = 0x1011
            class type = 0x1057, this type = 0x1058, this adjust = 0
            calling conv = cdecl, options = None

Also, if I use clang-cl and pass the -ast-dump option, it will look like this:

| |-CXXMethodDecl 0x23869a32c38 <line:2:3, col:25> col:8 simple_method 'void ()'
| | `-CompoundStmt 0x23869a336f0 <col:24, col:25>

So it doesn't have __attribute__((thiscall)) as you do in the test. Could you double check this?

33 ↗

(On Diff #179619)

Parameter names should only be important once you are debugging inside of a function, and at that point we will have already located the S_GPROC32 naturally. So I think it's ok for us to leave the parameter names blank, and update the AST lazily whenever we encounter an S_GPROC32 or S_LPROC32 naturally. We actually already parse the function parameter names in this method (See PdbAstBuilder::GetOrCreateFunctionDecl), so in fact I think everything will already mostly work, with only some minor code changes needed to support both global functions as well as member functions.

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

669–672 ↗

(On Diff #179619)

I think there is a problem here.

[aleksandr.urakov]

lit/SymbolFile/NativePDB/ast-methods.cpp

28 ↗

(On Diff #179619)

Hm... I tested it on x86, and I had there a "thiscall" attribute. It seems that on x64 it is not so. I think it's because the struct contains no fields and the methods doesn't use them - they don't need a "this" pointer. But I can't understand, why is there still "thiscall" on x86. Unfortunately, I can check it only two weeks later, but thanks for catching that!

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

669–672 ↗

(On Diff #179619)

I can't understand, why?..

[labath]

In D56126#1342114, @zturner wrote:

This info is contained in the Symbols stream, not in the TPI stream. As far as I understand, we can't find in a simple way the link between a LF_ONEMETHOD record and a corresponding S_GPROC32 record. I think it's the place, where we need to use the approach similar to D54053 (to parse mangled names and to build some part of AST based on this info). That's what I'm planning to implement next. Do you have any objections on this? Or may be there's a better way to solve this?

I think it's probably best to skip this part for now and come back to it later. The only thing that will be missing is the ability to use member function templates in expressions. Of course we need this eventually, but probably there is more useful stuff to work on first.

You might be interested to know that member function templates don't currently work in dwarf land either. I don't understand the full background, but the issue has something to do with the fact that they (and their debug info in dwarf) are emitted only when used, which means that the class declaration can end up being different depending on where you reconstruct it from. This leads to problems if two class definitions (e.g., from two different modules) end up being used in the same expression. There's a longer discussion about the problem here: http://lists.llvm.org/pipermail/llvm-dev/2018-June/124067.html

[aleksandr.urakov]

@zturner Sorry for the long delay with this update. I've applied the changes we were talking about.

@labath Thanks for sharing this, it's interesting to know!

[aleksandr.urakov]

Ping! Can you take a look, please?

[zturner]

Thanks for the reminder!

[aleksandr.urakov]

Thank you!

lldb/trunk/lit/SymbolFile/NativePDB/Inputs/ast-methods.lldbinit

+target variable s
+
+target modules dump ast
+
+quit

lldb/trunk/lit/SymbolFile/NativePDB/ast-methods.cpp

+// clang-format off
+// REQUIRES: lld
+
+// RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
+// RUN:     %p/Inputs/ast-methods.lldbinit 2>&1 | FileCheck %s
+
+struct Struct {
+  void simple_method() {}
+
+  virtual void virtual_method() {}
+
+  static void static_method() {}
+
+  int overloaded_method() {}
+  int overloaded_method(char c) {}
+  int overloaded_method(char c, int i, ...) {}
+};
+
+Struct s;
+
+int main(int argc, char **argv) {
+  return 0;
+}
+
+// CHECK: TranslationUnitDecl
+// CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
+// CHECK: | |-CXXMethodDecl {{.*}} simple_method 'void (){{.*}}'
+// CHECK: | |-CXXMethodDecl {{.*}} virtual_method 'void (){{.*}}' virtual
+// CHECK: | |-CXXMethodDecl {{.*}} static_method 'void ()' static
+// CHECK: | |-CXXMethodDecl {{.*}} overloaded_method 'int (){{.*}}'
+// CHECK: | |-CXXMethodDecl {{.*}} overloaded_method 'int (char){{.*}}'
+// CHECK: | | `-ParmVarDecl {{.*}} 'char'
+// CHECK: | `-CXXMethodDecl {{.*}} overloaded_method 'int (char, int, ...)'
+// CHECK: |   |-ParmVarDecl {{.*}} 'char'
+// CHECK: |   `-ParmVarDecl {{.*}} 'int'

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h

private:
   clang::QualType
   CreateModifierType(const llvm::codeview::ModifierRecord &modifier);
   clang::QualType CreateArrayType(const llvm::codeview::ArrayRecord &array);
   clang::QualType CreateRecordType(PdbTypeSymId id,
                                    const llvm::codeview::TagRecord &record);
   clang::QualType CreateEnumType(PdbTypeSymId id,
                                  const llvm::codeview::EnumRecord &record);
   clang::QualType
-  CreateProcedureType(const llvm::codeview::ProcedureRecord &proc);
+  CreateFunctionType(TypeIndex args_type_idx, TypeIndex return_type_idx,
+                     llvm::codeview::CallingConvention calling_convention);
   clang::QualType CreateType(PdbTypeSymId type);
 
   void CreateFunctionParameters(PdbCompilandSymId func_id,
                                 clang::FunctionDecl &function_decl,
                                 uint32_t param_count);
   clang::Decl *GetOrCreateSymbolForId(PdbCompilandSymId id);
   clang::VarDecl *CreateVariableDecl(PdbSymUid uid,
                                      llvm::codeview::CVSymbol sym,

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

if (cvt.kind() == LF_ARRAY) {
     ArrayRecord ar;
     llvm::cantFail(TypeDeserializer::deserializeAs<ArrayRecord>(cvt, ar));
     return CreateArrayType(ar);
   }
 
   if (cvt.kind() == LF_PROCEDURE) {
     ProcedureRecord pr;
     llvm::cantFail(TypeDeserializer::deserializeAs<ProcedureRecord>(cvt, pr));
-    return CreateProcedureType(pr);
+    return CreateFunctionType(pr.ArgumentList, pr.ReturnType, pr.CallConv);
+  }
+
+  if (cvt.kind() == LF_MFUNCTION) {
+    MemberFunctionRecord mfr;
+    llvm::cantFail(
+        TypeDeserializer::deserializeAs<MemberFunctionRecord>(cvt, mfr));
+    return CreateFunctionType(mfr.ArgumentList, mfr.ReturnType, mfr.CallConv);
   }
 
   return {};
 }
 
 clang::QualType PdbAstBuilder::GetOrCreateType(PdbTypeSymId type) {
   lldb::user_id_t uid = toOpaqueUid(type);
   auto iter = m_uid_to_type.find(uid);
clang::QualType PdbAstBuilder::CreateArrayType(const ArrayRecord &ar) {
   uint64_t element_count =
       ar.Size / GetSizeOfType({ar.ElementType}, m_index.tpi());
 
   CompilerType array_ct = m_clang.CreateArrayType(ToCompilerType(element_type),
                                                   element_count, false);
   return clang::QualType::getFromOpaquePtr(array_ct.GetOpaqueQualType());
 }
 
-clang::QualType
-PdbAstBuilder::CreateProcedureType(const ProcedureRecord &proc) {
+clang::QualType PdbAstBuilder::CreateFunctionType(
+    TypeIndex args_type_idx, TypeIndex return_type_idx,
+    llvm::codeview::CallingConvention calling_convention) {
   TpiStream &stream = m_index.tpi();
-  CVType args_cvt = stream.getType(proc.ArgumentList);
+  CVType args_cvt = stream.getType(args_type_idx);
   ArgListRecord args;
   llvm::cantFail(
       TypeDeserializer::deserializeAs<ArgListRecord>(args_cvt, args));
 
   llvm::ArrayRef<TypeIndex> arg_indices = llvm::makeArrayRef(args.ArgIndices);
   bool is_variadic = IsCVarArgsFunction(arg_indices);
   if (is_variadic)
     arg_indices = arg_indices.drop_back();
 
   std::vector<CompilerType> arg_types;
   arg_types.reserve(arg_indices.size());
 
   for (TypeIndex arg_index : arg_indices) {
     clang::QualType arg_type = GetOrCreateType(arg_index);
     arg_types.push_back(ToCompilerType(arg_type));
   }
 
-  clang::QualType return_type = GetOrCreateType(proc.ReturnType);
+  clang::QualType return_type = GetOrCreateType(return_type_idx);
 
   llvm::Optional<clang::CallingConv> cc =
-      TranslateCallingConvention(proc.CallConv);
+      TranslateCallingConvention(calling_convention);
   if (!cc)
     return {};
 
   CompilerType return_ct = ToCompilerType(return_type);
   CompilerType func_sig_ast_type = m_clang.CreateFunctionType(
       return_ct, arg_types.data(), arg_types.size(), is_variadic, 0, *cc);
 
   return clang::QualType::getFromOpaquePtr(

lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h

 #define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #include "llvm/DebugInfo/CodeView/CodeViewTypes.def"
 
   void complete();
 
 private:
   clang::QualType AddBaseClassForTypeIndex(llvm::codeview::TypeIndex ti,
                                            llvm::codeview::MemberAccess access);
+  void AddMethod(llvm::StringRef name, llvm::codeview::TypeIndex type_idx,
+                 llvm::codeview::MemberAccess access,
+                 llvm::codeview::MethodOptions options,
+                 llvm::codeview::MemberAttributes attrs);
 };
 
 } // namespace npdb
 } // namespace lldb_private
 
 #endif // LLDB_PLUGINS_SYMBOLFILE_NATIVEPDB_UDTRECORDCOMPLETER_H

lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp

std::unique_ptr<clang::CXXBaseSpecifier> base_spec =
       m_ast_builder.clang().CreateBaseClassSpecifier(
           qt.getAsOpaquePtr(), TranslateMemberAccess(access), false,
           udt_cvt.kind() == LF_CLASS);
   lldbassert(base_spec);
   m_bases.push_back(std::move(base_spec));
   return qt;
 }
 
+void UdtRecordCompleter::AddMethod(llvm::StringRef name, TypeIndex type_idx,
+                                   MemberAccess access, MethodOptions options,
+                                   MemberAttributes attrs) {
+  clang::QualType method_qt =
+      m_ast_builder.GetOrCreateType(PdbTypeSymId(type_idx));
+  m_ast_builder.CompleteType(method_qt);
+
+  lldb::AccessType access_type = TranslateMemberAccess(access);
+  bool is_artificial = (options & MethodOptions::CompilerGenerated) ==
+                       MethodOptions::CompilerGenerated;
+  m_ast_builder.clang().AddMethodToCXXRecordType(
+      m_derived_ct.GetOpaqueQualType(), name.data(), nullptr,
+      m_ast_builder.ToCompilerType(method_qt), access_type, attrs.isVirtual(),
+      attrs.isStatic(), false, false, false, is_artificial);
+}
+
 Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
                                            BaseClassRecord &base) {
   clang::QualType base_qt =
       AddBaseClassForTypeIndex(base.Type, base.getAccess());
 
   auto decl =
       m_ast_builder.clang().GetAsCXXRecordDecl(base_qt.getAsOpaquePtr());
   lldbassert(decl);
Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
 
   m_layout.field_offsets.insert(std::make_pair(decl, offset));
 
   return Error::success();
 }
 
 Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
                                            OneMethodRecord &one_method) {
+  AddMethod(one_method.Name, one_method.Type, one_method.getAccess(),
+            one_method.getOptions(), one_method.Attrs);
+
   return Error::success();
 }
 
 Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
                                            OverloadedMethodRecord &overloaded) {
+  TypeIndex method_list_idx = overloaded.MethodList;
+
+  CVType method_list_type = m_tpi.getType(method_list_idx);
+  assert(method_list_type.Type == LF_METHODLIST);
+
+  MethodOverloadListRecord method_list;
+  llvm::cantFail(TypeDeserializer::deserializeAs<MethodOverloadListRecord>(
+      method_list_type, method_list));
+
+  for (const OneMethodRecord &method : method_list.Methods)
+    AddMethod(overloaded.Name, method.Type, method.getAccess(),
+              method.getOptions(), method.Attrs);
+
   return Error::success();
 }
 
 Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
                                            EnumeratorRecord &enumerator) {
   Declaration decl;
   llvm::StringRef name = DropNameScope(enumerator.getName());