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

[Amy Huang via Phabricator via llvm-commits]

akhuang marked an inline comment as done.
akhuang added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:499
+      });
+  --LineIter;
+
----------------
labath wrote:
> akhuang wrote:
> > 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`. 
> Maybe something like:
> ```
> // This will find the first non-terminal entry whose address is >= VA
> auto LineIter = llvm::partition_point(Lines, [&] (const auto &E) { return E.Addr < VA || (E.Addr == VA && E.IsTerminalEntry); });
> if (LineIter == Lines.end() || LineIter->Addr > VA) {
>   // We've gone too far. Try to back up.
>   if (LineIter == Lines.begin() || std::prev(LineIter)->IsTerminalEntry) {
>     // Can't back up. It's not clear to me if the existing version handles the case where the `VA` would be in a "hole" between a previous terminal entry ending before VA and the next entry which starts after VA.
>     return nullptr;
>   }
>   --LineIter;
> }
> ```
oh, ok. this does look a lot simpler. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962