Page MenuHomePhabricator

[SymbolFilePDB] Add support for function symbols
ClosedPublic

Authored by asmith on Jan 23 2018, 2:58 PM.

Details

Summary

This is combination of following changes,

  • Resolve function symbols in PDB symbol file. lldb-test symbols will display information about function symbols.
  • Implement SymbolFilePDB::FindFunctions methods. On lldb console, searching function symbol by name and by regular expression are both available.
  • Create lldb type for PDBSymbolFunc.
  • Add tests to check whether functions with the same name but from different sources can be resolved correctly.

Diff Detail

Repository
rL LLVM

Event Timeline

asmith created this revision.Jan 23 2018, 2:58 PM
asmith edited the summary of this revision. (Show Details)Jan 23 2018, 3:00 PM
zturner added inline comments.Jan 31 2018, 1:35 PM
lit/SymbolFile/PDB/Inputs/FuncSymbols.cpp
2 ↗(On Diff #131146)

Would it be worth trying one in an anonymous namespace?

lit/SymbolFile/PDB/func-symbols.test
4 ↗(On Diff #131146)

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?

12 ↗(On Diff #131146)

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.

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
292 ↗(On Diff #131146)

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.

342 ↗(On Diff #131146)

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.

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
270–271 ↗(On Diff #131146)

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.

303–305 ↗(On Diff #131146)

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?

376–380 ↗(On Diff #131146)

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?

381–382 ↗(On Diff #131146)

This is another case that since we control all the inputs, I think we can either accept references or hard assert.

404–405 ↗(On Diff #131146)

can you do auto block_sp = std::make_shared<Block>(pdb_symbol->getSymIndexId());

549–550 ↗(On Diff #131146)

Did you mean to do something here?

561 ↗(On Diff #131146)

actual assert is ok here.

693–695 ↗(On Diff #131146)

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.

labath added a subscriber: labath.Feb 1 2018, 1:36 AM
labath added inline comments.
lit/SymbolFile/PDB/func-symbols.test
4 ↗(On Diff #131146)

That would be great, but you'd probably still need something like REQUIRES:lld, as lld is not currently not a mandatory requirement for building/testing.

asmith updated this revision to Diff 132785.Feb 4 2018, 7:37 PM
asmith marked 8 inline comments as done.Feb 4 2018, 7:43 PM
zturner accepted this revision.Feb 8 2018, 2:53 PM

Sorry for the delay on this one, looks good.

This revision is now accepted and ready to land.Feb 8 2018, 2:53 PM
asmith updated this revision to Diff 133561.Feb 8 2018, 9:31 PM

Minor cleanup before commit

asmith closed this revision.Feb 8 2018, 9:33 PM
This revision was automatically updated to reflect the committed changes.