[PATCH] D63104: Add GSYM utility files along with unit tests.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

A bunch of nits but this looks good to me.



================
Comment at: include/llvm/DebugInfo/GSYM/FunctionInfo.h:87
+/// when creating the final GSYM file. 
+inline bool operator<(const FunctionInfo &LHS, const FunctionInfo &RHS) {
+  // First sort by address range
----------------
nit: Most of the branches here are not tested by my eyes.


================
Comment at: include/llvm/DebugInfo/GSYM/FunctionInfo.h:98
+  if (LHS.Lines.size() == RHS.Lines.size())
+    return LHS.Lines < RHS.Lines;
+  // Then sort by number of line table entries (more is better)
----------------
The motivation for comparing by lines like this is a bit lost on me. Is it just to ensure that if X != Y then either X < Y or Y < X? You'd have to do that for inlines as well if that's the case. You also don't maintain that property on LineInfo anyway.


================
Comment at: include/llvm/DebugInfo/GSYM/LineEntry.h:33
+  bool isValid() { return File != 0; }
+  void dump(llvm::raw_ostream &OS) const {
+    OS << "addr=" << format("0x%08" PRIx64, Addr)
----------------
In other cases you forwent the dump method and overloaded <<, why not do that here as well?


================
Comment at: include/llvm/DebugInfo/GSYM/Range.h:40
+  bool contains(uint64_t Addr) const { return Start <= Addr && Addr < End; }
+  bool doesAdjoinOrIntersect(const AddressRange &RHS) const {
+    return (Start <= RHS.End) && (End >= RHS.Start);
----------------
maybe "isContiguousWith"


================
Comment at: include/llvm/DebugInfo/GSYM/Range.h:43
+  }
+  bool doesIntersect(const AddressRange &RHS) const {
+    return (Start < RHS.End) && (End > RHS.Start);
----------------
maybe "overlaps" or just "intersects" or "overlapsWith"


================
Comment at: include/llvm/DebugInfo/GSYM/Range.h:54
+}
+inline bool operator<(const AddressRange &LHS, const AddressRange &RHS) {
+  if (LHS.Start == RHS.Start)
----------------
If looking up the greatest lower bound on an address in an array sorted by this predicate, the largest range possible would be chosen. It seems to me that we would actually prefer the *smallest* range be chosen because its information would be more specific. That would suggest that instead of  `LHS.End < RHS.End` we should actually use `LHS.End > RHS.End`


================
Comment at: include/llvm/DebugInfo/GSYM/Range.h:68
+
+typedef std::vector<AddressRange> AddressRanges;
+bool contains(const AddressRanges &Ranges, uint64_t Addr);
----------------
prefer using


================
Comment at: include/llvm/DebugInfo/GSYM/StringTable.h:39
+  void clear() { Data = StringRef(); }
+  void dump(raw_ostream &OS) const {
+    OS << "String table:\n";
----------------
same question


================
Comment at: include/llvm/DebugInfo/GSYM/StringTable.h:49
+  }
+  llvm::Optional<uint32_t> find(StringRef Str) const {
+    // Return the first byte of the GSYM string table which contains the 
----------------
What is this needed for? I'm not sure I'm happy with this sort of linear search being needed.


================
Comment at: lib/DebugInfo/GSYM/InlineInfo.cpp:41
+
+bool InlineInfo::getInlineStack(
+    uint64_t Addr, std::vector<const InlineInfo *> &InlineStack) const {
----------------
return Option<std::vector<...>> instead perhaps?


================
Comment at: lib/DebugInfo/GSYM/Range.cpp:22
+void llvm::gsym::insert(AddressRanges &Ranges, const AddressRange &Range) {
+  Ranges.insert(std::upper_bound(Ranges.begin(), Ranges.end(), Range), Range);
+}
----------------
Do we expect the number of ranges to be small?


================
Comment at: lib/DebugInfo/GSYM/Range.cpp:26-31
+  if (Ranges.empty())
+    return false;
+  if (Addr < Ranges.front().Start)
+    return false;
+  if (Addr >= Ranges.back().End)
+    return false;
----------------
None of this is needed and should fallout from the std::upper_bound call


================
Comment at: lib/DebugInfo/GSYM/Range.cpp:32
+    return false;
+  auto begin = Ranges.begin();
+  auto EndPos = Ranges.end();
----------------
s/begin/Begin


================
Comment at: lib/DebugInfo/GSYM/Range.cpp:34
+  auto EndPos = Ranges.end();
+  auto Pos = std::upper_bound(begin, EndPos, Addr);
+  if (Pos == EndPos)
----------------
In general I think you want a greatest lower bound which is what you're doing but if you keep your current '<' operator and ignore my comment above about swaping the order of 'End' you can use `std::lower_bound` here and avoid the subtraction case.


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

https://reviews.llvm.org/D63104