[PATCH] D143793: Add the ability to segment GSYM files.

[jeffrey tan via Phabricator via llvm-commits]

yinghuitan added inline comments.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:454
+  // Add size of address info offsets
+  size += NumFuncs * 4;
+  // Add file table size
----------------
Instead of magic value 4, maybe `sizeof(<type-of-offset-entry>)` in case it is changed in future?


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:503-505
+  std::lock_guard<std::mutex> Guard(Mutex);
+  Funcs.push_back(DstFI);
+  return Funcs.back().cacheEncoding();
----------------
Is the lock only needed during accessing `Funcs` field? Maybe scope locking it instead of locking the entire function.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:517-518
+    std::unique_ptr<GsymCreator> GC = createSegment(SegmentSize, FuncIdx);
+    if (!GC)
+      break;
+    raw_null_ostream ErrorStrm;
----------------
Shouldn't this be an error with meaning message instead of simply break and return success? 


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:555
+  for (; FuncIdx < NumFuncs; ++FuncIdx) {
+    const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+    if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
----------------
This is not changing per function, right? If so move outside of loop.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:556
+    const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+    if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
+      break;
----------------
We should also detect outside loop at least `SegmentSize > SegmentFuncInfosSize` so that we are not creating dummy segment file with only header. 


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:322
+
+  if (auto Err = Gsym.save(OutFile, Endian, SegmentSize > 0 ? std::optional(SegmentSize.getValue()) : std::nullopt))
     return Err;
----------------
The line is very long, maybe run clang-format against it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143793