[PATCH] D80962: [NativeSession] Implement findLineNumbersByAddress functions for native llvm-symbolizer support.

Thu Jun 4 17:41:46 PDT 2020

akhuang added a comment.

@labath thanks for the comments! I removed some of the autos (but probably missed a bunch); I agree about the readability thing.

================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:473-477
+        // Otherwise, insert where it belongs.
+        auto Pos =
+            upper_bound(LineTable, EntryAddr, [](uint64_t Addr, auto &Entry) {
+              return Addr < Entry.Addr;
+            });
----------------
labath wrote:
> I'm not sure how often can this happen in PDBs, but in DWARF it happens a lot. A pattern like this has caused a significant slowdown in lldb in the past. It might be better to replace this with a better data structure. For instance you could store the data in a vector of vectors, sort it at the end of the function, and only flatten as the very last step. (That's what we did in lldb.)
Good to know. I'll try doing this.

================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:499
+      });
+  --LineIter;
+
----------------
labath wrote:
> This assumes that the table contains at least one element, but I'm not sure if that is always the case. To an unfamiliar reader it does seem like `populateLineTable` can legitimately return without inserting anything into the table.
> 
> I also get a distinct feeling that this iterator manipulation could be simplified, but I'm not sure how because I'm not exactly sure what is this code doing.
My bad, I meant to put another non-empty check after calling `populateLineTable`. 

Maybe it can be simplified; it's searching for the first address in the line table that is not greater than `VA`. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962