This is an archive of the discontinued LLVM Phabricator instance.

Retrieve a function PDB symbol correctly from nested blocks
ClosedPublic

Authored by aleksandr.urakov on Jun 8 2018, 6:10 AM.

Details

Summary

This patch fixes a problem with retrieving a function symbol by an address in a nested block. In the current implementation of ResolveSymbolContext function it retrieves a symbol with PDB_SymType::None and then checks if found symbol's tag equals to PDB_SymType::Function. So, if nested block's symbol was found, ResolveSymbolContext does not resolve a function.

It is very simple to reproduce this. For example, in the next program

int main() {
  auto r = 0;
  for (auto i = 1; i <= 10; i++) {
    r += i & 1 + (i - 1) & 1 - 1;
  }

  return r;
}

if we will stop inside the cycle and will do a backtrace, the top element will be broken. But how we can test this? I thought to add an option to lldb-test to allow search a function by address, but the address may change when the compiler will be changed.

Diff Detail

Event Timeline

aleksandr.urakov created this revision.
labath added a comment.Jun 8 2018, 8:37 AM

Yea, relying on addresses inside compiler-generated binaries would be extremely brittle. However, line numbers should be fairly stable. Maybe we could have lldb-test take an file+line argument, resolve that into an address, and then look up the relevant symbol context? As a bonus we also get to test line number resolution :P.

Would that work for your needs?

Yea, relying on addresses inside compiler-generated binaries would be extremely brittle. However, line numbers should be fairly stable. Maybe we could have lldb-test take an file+line argument, resolve that into an address, and then look up the relevant symbol context? As a bonus we also get to test line number resolution :P.

Would that work for your needs?

Yes, it's a good idea, thank you!

But lldb used DWARF-like (one based) files numeration, so resolving file+line into an address didn't work. I have fixed that too in the patch.

asmith requested changes to this revision.Jun 10 2018, 1:04 PM

Few small comments otherwise LGTM.

tools/lldb-test/lldb-test.cpp
151

Would "Search source file for function" be a better description?

153

Would "Search source file by line number and function" be a better description?

541

Would you move the opening brace '{' to the end of the previous line and the ones following.

This revision now requires changes to proceed.Jun 10 2018, 1:04 PM

Looks great. Thanks.

For the formatting, I'd recommend getting into the habit of running clang-format over your change before upload.

Hello!

Excuse me for a long delay with the reply, I had a vacation.

I have fixed mentioned problems in the patch, the only thing is that I have written "Line to search." description for the -line parameter (by analogy with the -file parameter, which Pavel has added). Do not you mind?

labath accepted this revision.Jun 27 2018, 4:23 AM

This looks fine to me, but let's also wait for Aaron.

asmith accepted this revision.Jun 27 2018, 5:03 PM
This revision is now accepted and ready to land.Jun 27 2018, 5:03 PM

Thank you!

Please, commit this, because I have no commit access.

This revision was automatically updated to reflect the committed changes.

This test is actually failing on Windows with the Visual Studio Generator and the latest DIA SDK:

E:\_work\1\s\llvm\tools\lldb\lit\SymbolFile\PDB\function-nested-block.test(7,8): error G8E235623: expected string not found in input [E:\_work\1\b\LLVMBuild\tools\lldb\lit\check-lldb-lit.vcxproj]
          
          CHECK: name = "{{.*}}", mangled = "_main"
          
                 ^
          
          <stdin>:3:1: note: scanning from here
          
          0x000000e40bf8f180: SymbolContextList
          
          ^
          
          <stdin>:6:31: note: possible intended match here
          
           Function: id = {0x00000003}, name = "main", mangled = "main", range = function-nested-block.test.tmp.exe[0x0000000140001000-0x000000014000105b)

Thanks for catching that!

I have attached a diff with a fix for the test.

I can confirm that this fixes the test. I just ran the tests on Windows in our setup and it passed. Thanks!

Please, commit this for me, because I have no commit access.