Page MenuHomePhabricator

Add lookup functions for efficient lookups of addresses when using GsymReader classes.
ClosedPublic

Authored by clayborg on Dec 3 2019, 4:58 PM.

Details

Summary

Lookup functions are designed to not fully decode a FunctionInfo, LineTable or InlineInfo, they decode only what is needed into a LookupResult object. This allows lookups to avoid costly memory allocations and avoid parsing large amounts of information one a suitable match is found.

LookupResult objects contain the address that was looked up, the concrete function address range, the name of the concrete function, and a list of source locations. One for each inline function, and one for the concrete function. This allows one address to turn into multiple frames and improves the signal you get when symbolicating addresses in GSYM files.

Diff Detail

Event Timeline

clayborg created this revision.Dec 3 2019, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2019, 4:58 PM
labath resigned from this revision.Dec 4 2019, 1:13 AM

The code reads ok, given that I have very little knowledge of how gsym actually works. However, overall, I don't feel qualified to review patches in this area.

llvm/lib/DebugInfo/GSYM/InlineInfo.cpp
76–108

It looks like these functions should be static

llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp
1353

So, if there's a bug here, then the entire test will just succeed? It looks like it would be better to ASSERT that this succeeded?

1356

ASSERT_THAT_EXPECTED(LR, Succeeded()) would be better as it would actually print the error message if it failed.

1373–1381

I'd consider defining operator== and << on SourceLocation and then writing this as EXPECT_THAT(LR->Locations, testing::ElementsAre(SourceLocation{"inline1", 10u, "/tmp", "foo.h"}, SourceLocation{"main", 6u, "/tmp", "main.c"}));

The interfaces seem fine.

llvm/lib/DebugInfo/GSYM/LookupResult.cpp
6

wrong license again

clayborg updated this revision to Diff 232215.Dec 4 2019, 2:31 PM

Fixed:

  • Updates LLVM license
  • Added gsym::SourceLocation operator==
  • Added gsym::SourceLocation operator<< for raw_ostream
  • Change gsym::LookupResult::dump to operator<< for raw_ostream
  • Converted tests to use testing::ElementsAre to make tests easier to read
clayborg marked 3 inline comments as done.Dec 4 2019, 2:33 PM
clayborg added inline comments.
llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp
1353

This was a copy/paste error from older code updated it to match other tests.

clayborg updated this revision to Diff 232216.Dec 4 2019, 2:40 PM

Change over to using ASSERT_THAT_EXPECTED(LR, Succeeded())

aprantl accepted this revision.Dec 4 2019, 5:06 PM
This revision is now accepted and ready to land.Dec 4 2019, 5:06 PM
This revision was automatically updated to reflect the committed changes.

This broke the Windows LLDB Buildbot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/13723

It already had a test failure, so you probably didn't get the notification.