[PATCH] D42443: [SymbolFilePDB] Add support for function symbols

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lit/SymbolFile/PDB/Inputs/FuncSymbols.cpp:2
+// Static function
+static long StaticFunction(int a)
+{
----------------
Would it be worth trying one in an anonymous namespace?


================
Comment at: lit/SymbolFile/PDB/func-symbols.test:4
+RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/FuncSymbols.cpp /o %T/FuncSymbols.cpp.obj
+RUN: link %T/FuncSymbolsTestMain.cpp.obj %T/FuncSymbols.cpp.obj /DEBUG /nodefaultlib /Entry:main /OUT:%T/FuncSymbolsTest.exe
+RUN: lldb-test symbols %T/FuncSymbolsTest.exe | FileCheck %s
----------------
I bet we could get rid of `REQUIRES: windows` if we changed this to `lld-link`.  You're already specifying `/nodefaultlib /entry:main`, and no windows header files are included, so maybe it's worth a try?


================
Comment at: lit/SymbolFile/PDB/func-symbols.test:12
+CHECK-DAG: {{.*}}: SymbolVendor ([[MD]])
+CHECK-DAG: [[TY0:.*]]:   Type{[[UID0:.*]]} , name = "Func_arg_array", decl = FuncSymbolsTestMain.cpp:3, compiler_type = {{.*}} int (int *)
+CHECK-DAG: [[TY1:.*]]:   Type{[[UID1:.*]]} , name = "Func_arg_void", decl = FuncSymbolsTestMain.cpp:4, compiler_type = {{.*}} void (void)
----------------
Not required, but just as a note, you can do 

```
CHECK-DAG: [[TY0:.*]]
CHECK-SAME: name = "Func_arg_array"
CHECK-SAME: decl=FuncSymbolsTestMain.cpp:3

CHECK-DAG: [[TTY1:.*]]
```

etc if the long lines bothers you.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:292
+    } else
+      return nullptr;
+
----------------
You can probably put an `llvm_unreachable` here.  If it has one of those 2 enum values, it had better cast to one type or the other.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:342
+    auto array_type = llvm::dyn_cast<PDBSymbolTypeArray>(&type);
+    lldbassert(array_type);
     uint32_t num_elements = array_type->getCount();
----------------
I think these can all be real asserts.  Like before, if it has the proper enum value, it really should cast.  And we're about to dereference it anyway, so no harm no foul.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:270-271
+    const lldb_private::SymbolContext &sc) {
+  if (!pdb_func)
+    return nullptr;
+  lldbassert(sc.comp_unit && sc.module_sp.get());
----------------
Can we either change `pdb_func` to be a reference, or assert that it's non null?  We have complete control over all entry points to this function, so we should be safe to make the assumption about the inputs.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:303-305
+  if (func_sp.get() != nullptr) {
+    sc.comp_unit->AddFunction(func_sp);
+  }
----------------
This null check isn't necessary, since you just unconditionally newed it.  Also, in the above line, can we use `make_shared` instead of `reset`?


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:376-380
+SymbolFilePDB::ParseFunctionBlocks(const lldb_private::SymbolContext &sc,
+                                   uint64_t func_file_vm_addr,
+                                   const llvm::pdb::PDBSymbol *pdb_symbol,
+                                   lldb_private::Block *parent_block,
+                                   bool is_top_parent) {
----------------
This function does not appear to access any member data.  Can you remove it from the class and mark it `static` at the file level?


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:381-382
+                                   bool is_top_parent) {
+  if (!pdb_symbol || !parent_block)
+    return 0;
+
----------------
This is another case that since we control all the inputs, I think we can either accept references or hard assert.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:404-405
+
+      block = new Block(pdb_symbol->getSymIndexId());
+      BlockSP block_sp(block);
+      parent_block->AddChild(block_sp);
----------------
can you do `auto block_sp = std::make_shared<Block>(pdb_symbol->getSymIndexId());`


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:549-550
+    if (!cu_sp) {
+      if (resolved_flags | eSymbolContextVariable) {
+      }
+      return 0;
----------------
Did you mean to do something here?


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:561
+        auto *pdb_func = llvm::dyn_cast<PDBSymbolFunc>(symbol_up.get());
+        lldbassert(pdb_func);
+        auto func_uid = pdb_func->getSymIndexId();
----------------
actual `assert` is ok here.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:693-695
+      if (resolve_scope & (eSymbolContextFunction | eSymbolContextBlock |
+                           eSymbolContextLineEntry)) {
+        if (ParseCompileUnitLineTable(sc, line)) {
----------------
Can you refactor this block a bit to include a bunch of early continues and/or breaks?  The indentation level gets a little deep as you go down.


Repository:
  rL LLVM

https://reviews.llvm.org/D42443