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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 2 inline comments as done.
rnk added inline comments.


================
Comment at: lld/COFF/PDB.cpp:191
+  case SymbolKind::S_THUNK32:
+  case SymbolKind::S_INLINESITE:
+    return true;
----------------
zturner wrote:
> zturner wrote:
> > 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)`
> s/if we're not supporting code/if we're not supporting **managed** code/
I went ahead and added the non-managed ones you mentioned.


================
Comment at: lld/COFF/PDB.cpp:223
+                           uint32_t CurOffset,
+                           MutableArrayRef<uint8_t> Contents) {
+  SymbolScope S;
----------------
zturner wrote:
> 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.
Sure, but then I have to add a const_cast.


================
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());
 
----------------
zturner wrote:
> Should we do something if `Scopes.empty()`?  Warn, assert, ignore, drop the record?
Warn, I guess.


================
Comment at: lld/test/COFF/pdb-scopes.test:40-42
+CHECK: - S_FRAMEPROC [size = 32]
+CHECK: - S_REGREL32 [size = 16] `x`
+CHECK:     type = 0x0074 (int), register = rsp, offset = 8
----------------
zturner wrote:
> Remove these from the test as they aren't relevant?
I removed the guts of the S_REGREL32 records.


================
Comment at: lld/test/COFF/pdb-scopes.test:43
+CHECK:     type = 0x0074 (int), register = rsp, offset = 8
+CHECK: - S_END [size = 4]
+CHECK: - S_GPROC32_ID [size = 44] `main`
----------------
zturner wrote:
> I wonder if we should display the offset of each symbol in the output.  For types we write something like:
> 
> ```
>  0x1017 | LF_CLASS [size = 60]
>            class name: `MembersTest::A`
>            unique name: `.?AVA at MembersTest@@`
>            vtable: <no type>, base list: <no type>, field list: <no type>
>            options: forward ref | has unique name
> ```
> 
> But we have no such analogue for symbols.  The only way to uniquely identify a symbol is by its offset in the symbol stream, but since this is what the `Parent` and `End` fields do anyway, perhaps we could display it?  This would also allow the test to be more robust, as you could say:
> 
> ```
> CHECK: -    0 | S_GPROC32_ID [size = 44] `g`
> CHECK:            parent = 0, addr = 0002:0000, code size = 5, end = [[END:r[0-9]+]]
> CHECK:            debug start = 4, debug end = 4, flags = none
> CHECK: -  [[END]] | S_END [size = 4]
> ```
> 
> Right now we're verifying that we're writing //some number// for the field, but we're not verifying that it's the //correct// number
Now that you've extended the dumper, this is done.


================
Comment at: lld/test/COFF/pdb-scopes.test:57-58
+CHECK: - S_BLOCK32 [size = 24] ``
+CHECK:     parent = 200, addr = 0002:0050
+CHECK:     code size = 17, end = 380
+CHECK: - S_REGREL32 [size = 16] `y`
----------------
zturner wrote:
> This ties into what I was saying earlier, but it's not clear to me what this number `200` represents.  It doesn't seem to be the offset of any symbol.  (I did the math by hand, and the offsets are:
> 
> ```
> S_GPROC32     0
> S_FRAMEPROC   44
> S_REGREL32    76
> S_END         92
> S_GPROC32_ID  96
> S_FRAMEPROC   140
> S_REGREL32    172
> S_BLOCK32     192
> S_REGREL32    216
> S_END         232
> S_BLOCK32     240
> ```
> 
> Nothing has offset 200.  So what is this number?
It should be clear now that the enclosing S_GPROC32_ID has that offset. There are some TU-wide S_ records that I deleted because they weren't relevant.


https://reviews.llvm.org/D34898