[PATCH] D70993: Add lookup functions for efficient lookups of addresses when using GsymReader classes.

[Pavel Labath via Phabricator via llvm-commits]

labath resigned from this revision.
labath added a comment.

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.



================
Comment at: llvm/lib/DebugInfo/GSYM/InlineInfo.cpp:77-109
+bool skip(DataExtractor &Data, uint64_t &Offset, bool SkippedRanges) {
+  if (!SkippedRanges) {
+    if (AddressRanges::skip(Data, Offset) == 0)
+      return false;
+  }
+  bool HasChildren = Data.getU8(&Offset) != 0;
+  Data.getU32(&Offset); // Skip Inline.Name.
----------------
It looks like these functions should be static


================
Comment at: llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp:1351
+  ASSERT_FALSE((bool)Err);
+  if (auto Gsym = GsymReader::copyBuffer(OutStrm.str())) {
+    // Verify inline info is correct when doing lookups.
----------------
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? 


================
Comment at: llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp:1354
+    auto LR = Gsym->lookup(0x1000);
+    ASSERT_TRUE(bool(LR));
+    ASSERT_EQ(LR->Locations.size(), 1u);
----------------
`ASSERT_THAT_EXPECTED(LR, Succeeded())` would be better as it would actually print the error message if it failed.


================
Comment at: llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp:1371-1379
+    ASSERT_EQ(LR->Locations.size(), 2u);
+    EXPECT_EQ(LR->Locations[0].Name, "inline1");
+    EXPECT_EQ(LR->Locations[0].Line, 10u);
+    EXPECT_EQ(LR->Locations[0].Dir, "/tmp");
+    EXPECT_EQ(LR->Locations[0].Base, "foo.h");
+    EXPECT_EQ(LR->Locations[1].Name, "main");
+    EXPECT_EQ(LR->Locations[1].Line, 6u);
----------------
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"}));`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70993/new/

https://reviews.llvm.org/D70993