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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I'm afraid I lack sufficient context to be able to give meaningful feedback here. Right now all I can to is complain that the use of `auto` in the patch, while being consistent with the other code in these files, is probably not consistent with the llvm guidelines on `auto` usage. At least for me the `auto` does not make this code more readable as it obscures the types of everything, including some fairly important details like whether something is an `Optional<T>` or `Expected<T>`. I don't really have a specific suggestion on that. Just saying...



================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:1-2
+//==- NativeEnumLineNumbers.cpp - Native Type Enumerator impl ----------*- C++
+//-*-==//
+//
----------------
bad formatting


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeLineNumber.cpp:1-2
+//===- NativeLineNumber.cpp - Native implementation of IPDBLineNumber -*- C++
+//-*-===//
+//
----------------
here too


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSourceFile.cpp:37
+std::string NativeSourceFile::getChecksum() const {
+  return (const char *)Checksum.Checksum.data();
+}
----------------
This looks suspicious. Is the checksum really guaranteed to be null terminated? Maybe something like `toStringRef(Checksum.Checksum).str()` ?


================
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;
+            });
----------------
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.)


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:499
+      });
+  --LineIter;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962