[Lldb-commits] [PATCH] D56126: [NativePDB] Add basic support of methods recostruction in AST

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lit/SymbolFile/NativePDB/ast-methods.cpp:28
+// CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
+// CHECK: | |-CXXMethodDecl {{.*}} simple_method 'void () __attribute__((thiscall))'
+// CHECK: | |-CXXMethodDecl {{.*}} virtual_method 'void () __attribute__((thiscall))' virtual
----------------
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?


================
Comment at: lit/SymbolFile/NativePDB/ast-methods.cpp:33
+// CHECK: | |-CXXMethodDecl {{.*}} overloaded_method 'int (char) __attribute__((thiscall))'
+// CHECK: | | `-ParmVarDecl {{.*}} 'char'
+// CHECK: | `-CXXMethodDecl {{.*}} overloaded_method 'int (char, int, ...)'
----------------
aleksandr.urakov wrote:
> 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?
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.


================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:669-672
+    MemberFunctionRecord mfr;
+    llvm::cantFail(
+        TypeDeserializer::deserializeAs<MemberFunctionRecord>(cvt, mfr));
+    return CreateFunctionType(mfr.ArgumentList, mfr.ReturnType, mfr.CallConv);
----------------
I think there is a problem here.  


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56126/new/

https://reviews.llvm.org/D56126