[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:268-269
+
+std::unique_ptr<llvm::pdb::PDBSymbol>
+GetClassOrFunctionParent(const llvm::pdb::PDBSymbol &symbol) {
+  const IPDBSession &session = symbol.getSession();
----------------
All file local functions should be marked `static`.


================
Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:273-288
+  auto class_parent_id = raw.getClassParentId();
+  if (auto class_parent = session.getSymbolById(class_parent_id))
+    return class_parent;
+
+  auto lexical_parent_id = raw.getLexicalParentId();
+  auto lexical_parent = session.getSymbolById(lexical_parent_id);
+  if (!lexical_parent)
----------------
This function is a little too general because it hits the `IPDBRawSymbol` interface which I would strongly discourage, and as a result I think it's probably wrong.  class parent id only valid for certain types of symbols in the first place.  Likewise, `getLexicalParentId()` is only valid for certain types of symbols.  The full list is:



| SymbolType | LexicalParent | ClassParent |
| Array | Compiland | <invalid> |
| BaseClass | Compiland | Enclosing Class |
| BuiltinType | Compiland |  <invalid> |
| Enum | Compiland | Enclosing Class |
| FunctionArg | Compiland | Enclosing Function |
| FunctionType | Compiland | Enclosing Class |
| Pointer | Compiland | <invalid> |
| UDT | Compiland | Class Parent  |
| VTable | Compiland | Class Parent |
| VTableShape | Compiland | <invalid> |
| Compiland | Exe File | <invalid> |
| CompilandDetails | Compiland | <invalid> |
| CompilandEnv | Compiland | <invalid> |
| Data | Compiland, Function, or Block | Enclosing Class (or null) |
| Exe | <invalid> | <invalid> |
| Function | Compiland | Class Parent (if member function) |
| FunctionDebugStart | Function | <invalid> |
| FunctionDebugEnd | Function | <invalid> |
| PublicSymbol | Exe | <invalid> |
| Thunk | Compiland or Function | <invalid> |



================
Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:289
+  return GetClassOrFunctionParent(*lexical_parent);
+}
+
----------------
However, this is only for symbols (not types).  So we can exclude everything above Compiland.  In fact, we should `assert` that we don't get anything from above Compiland.  So I would write this as:


```
uint32_t ParentId = 0;
switch (raw.getSymTag()) {
case PDB_SymType::Array:
case PDB_SymType::BaseClass:
case PDB_SymType::BuiltinType:
case PDB_SymType::Enum:
case PDB_SymType::FunctionArg:
case PDB_SymType::FunctionType:
case PDB_SymType::PointerType:
case PDB_SymType::UDTType:
case PDB_SymType::VTable:
case PDB_SymType::VTableShape:
  assert(false && "This is a type not a symbol!");
  return nullptr;
default:
  break;
}
```

And for the rest it's quite simple because you probably really only care about functions, public symbols, and data.  In which case you can continue:

template<typename T>
static std::unique_ptr<llvm::pdb::PDBSymbol>
classOrLexicalParent(const T& t, IPDBSession &Session) {
  uint32_t parent = t.getClassParentId();
  if (parent == 0)
    parent = t.getLexicalParentId();
  return Session.getSymbolById(parent);
}

```
if (auto &F = dyn_cast<PDBSymbolFunc>(symbol)) {
  return classOrLexicalParent(F);
} else if (auto &D = dyn_cast<PDBSymbolData>(symbol)) {
  return classOrLexicalParent(F);
} else if (auto &P = dyn_cast<PDBSymbolPublic>(symbol)) {
  return P.getLexicalParent();
}

```


Repository:
  rL LLVM

https://reviews.llvm.org/D51162