[PATCH] D34898: [PDB] Fill in "Parent" and "End" fields of scope-like symbol records

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lld/COFF/PDB.cpp:191
+  case SymbolKind::S_THUNK32:
+  case SymbolKind::S_INLINESITE:
+    return true;
----------------
I found a couple of additional symbols that I wonder if we need to handle.

From cvdump.h
```
    // Separated code (from the compiler) support
    S_SEPCODE       =  0x1132,
    
    // extended inline site information
    S_INLINESITE2 = 0x115d,
```

We don't currently handle either of these and I've never seen a problem, but maybe you have some theories on why they might arise?

Also, if we're not supporting code (we aren't), then might as well delete `S_WITH32`.  Otherwise you might as well add all the other managed symbols `(e.g. S_MANPROC)`


================
Comment at: lld/COFF/PDB.cpp:217
+struct SymbolScope {
+  ScopeRecord *Rec;
+  uint32_t ScopeOffset;
----------------
Can you change the name of this to somehow indicate that this is the begin or opening symbol of the scope?  It's probably obvious but a better name would make it even more obvious.


================
Comment at: lld/COFF/PDB.cpp:223
+                           uint32_t CurOffset,
+                           MutableArrayRef<uint8_t> Contents) {
+  SymbolScope S;
----------------
Can you change the signature to

```
static void scopeStackOpen(SmallVectorImpl<SymbolScope> &Stack,
                           uint32_t Offset,
                           CVSymbol &BeginSymbol) {
```
and then add an assert on line 1:

```
assert(symbolOpensScope(BeginSymbol.kind()));
```

Mostly just for readability.


================
Comment at: lld/COFF/PDB.cpp:267-268
+      scopeStackOpen(Scopes, File->ModuleDBI->getNextSymbolOffset(), Contents);
+    else if (symbolEndsScope(Sym.kind()) && !Scopes.empty())
+      scopeStackClose(Scopes, File->ModuleDBI->getNextSymbolOffset());
 
----------------
Should we do something if `Scopes.empty()`?  Warn, assert, ignore, drop the record?


https://reviews.llvm.org/D34898