This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rnk on Jun 30 2017, 10:26 AM.

Details

Summary

There are a variety of records that open scopes: function scopes, block
scopes, and inlined call site scopes. These symbol records contain
Parent and End fields with the offsets of other symbol records. The End
field contains the offset of the matching S_END or S_INLINESITE_END
record. The Parent field contains the offset of the parent record, or 0
if this is a top-level scope (i.e. a function).

With this change, llvm-pdbutil pretty -all no longer crashes on PDBs
produced by LLD. I haven't tried a real debugger yet.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Jun 30 2017, 10:26 AM
zturner added inline comments.Jun 30 2017, 11:42 AM
lld/test/COFF/pdb-scopes.test
38 ↗(On Diff #104892)

Would it be helpful to format this (and similar) records so that parent = 0, end = 196 is at the beginning of the string, so that you can just match that and leave out the rest of the (relatively fragile) debug info which we're not trying to test?

40–42 ↗(On Diff #104892)

Remove these from the test as they aren't relevant?

43 ↗(On Diff #104892)

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@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

57–58 ↗(On Diff #104892)

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?

zturner added inline comments.Jun 30 2017, 2:26 PM
lld/COFF/PDB.cpp
191 ↗(On Diff #104892)

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)

217 ↗(On Diff #104892)

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.

223 ↗(On Diff #104892)

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.

267–268 ↗(On Diff #104892)

Should we do something if Scopes.empty()? Warn, assert, ignore, drop the record?

zturner added inline comments.Jun 30 2017, 2:27 PM
lld/COFF/PDB.cpp
191 ↗(On Diff #104892)

s/if we're not supporting code/if we're not supporting managed code/

rnk marked 2 inline comments as done.Jun 30 2017, 3:46 PM
rnk added inline comments.
lld/COFF/PDB.cpp
191 ↗(On Diff #104892)

I went ahead and added the non-managed ones you mentioned.

223 ↗(On Diff #104892)

Sure, but then I have to add a const_cast.

267–268 ↗(On Diff #104892)

Warn, I guess.

lld/test/COFF/pdb-scopes.test
40–42 ↗(On Diff #104892)

I removed the guts of the S_REGREL32 records.

43 ↗(On Diff #104892)

Now that you've extended the dumper, this is done.

57–58 ↗(On Diff #104892)

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.

zturner edited edge metadata.Jun 30 2017, 5:02 PM

Did the new patch not get uploaded? I'm still seeing the original patch.

lld/COFF/PDB.cpp
223 ↗(On Diff #104892)

Oh, yea nevermind then.

rnk updated this revision to Diff 105435.Jul 6 2017, 8:22 AM
  • comments
zturner accepted this revision.Jul 6 2017, 9:01 AM
This revision is now accepted and ready to land.Jul 6 2017, 9:01 AM
This revision was automatically updated to reflect the committed changes.